Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add fortify.DataFrame #116

Closed
wants to merge 2 commits into from
Closed

Conversation

kevinrue
Copy link

Hi there

TL;DR: This PR makes the following code possible:

library(S4Vectors)

df <- DataFrame(
  x = 1:10,
  y = rnorm(10)
)

library(ggplot2)

ggplot(df, aes(x, y)) +
  geom_point()

# see also:
fortify(df)

Without the need for as.data.frame(df) in the ggplot() call.


Context:

As per https://community-bioc.slack.com/archives/C6KJHH0M9/p1690235172607369

I've messed around S4vectors a bit to test feasibility, and somehow landed on my feet with something that seems to work.

I'll be honest, I'm not even sure why R allows me to do it, but it seems that I can importFrom a package that is listed in Suggests (i.e., not in Depends).

I added ggplot2 to Suggests because I don't like the idea of having it under Imports. It just feels wrong to automatically install ggplot2 and its own dependencies as a dependency of S4Vectors. S4Vectors should remain a lightweight package.

I suppose that if users have ggplot2 installed, the import statement "just works", and if they don't have ggplot2 installed.. well... they don't have any reason to call ggplot() on a DataFrame object :D

I'm aware that this PR is unlikely to be the final fix (if any is possible at all). I just aim to give a starting point to the discussion.

Also, I've considered other approaches, but run into chicken-and-egg issues:

  • I suspect ggplot2 will not accept to Suggests: S4Vectors, as I don't see any Bioconductor package in its existing Imports/Suggests (https://cran.r-project.org/web/packages/ggplot2/index.html) and install.packages() cannot see Bioconductor packages without messing with options(repos).
  • I suspect S4Vectors will not accept to Imports: ggplot2, to justify more cleanly importFrom(ggplot2, fortify). Same reason as above: keep S4Vectors dependencies to a minimum
  • I noted that ?ggplot2::fortify states "Rather than using this function, I now recommend using the broom package, which implements a much wider range of methods. fortify() may be deprecated in the future." However, it is not clear to me what needs to be done in broom (or biobroom)

@antagomir
Copy link

Thanks @kevinrue - this is also a frequent source of confusion in training events for instance, finding a solution would be great.

@hpages
Copy link
Contributor

hpages commented Aug 11, 2023

Hi Kevin,

Thanks for the PR.

I would prefer the cleaner and more general solution I suggested in the community-bioc Slack. Copying it here for reference:

Does anybody know why their default fortify() method (ggplot2:::fortify.default()) doesn't simply try to call as.data.frame() on the supplied object? Might be worth asking them. Would make ggplot() work on any object that supports as.data.frame() e.g. DataFrame, matrix, dgCMatrix, DelayedMatrix, SparseMatrix, etc... Why add the additional requirement that these objects must implement a fortify() method when all that is needed is that they support as.data.frame()?

Of course we have no control on that, so, if for whatever reason this doesn't work, then we'll add fortify.DataFrame to S4Vectors.

Thanks,
H.

@antagomir
Copy link

Ok I opened this issue in ggplot now:
tidyverse/ggplot2#5390

@teunbrand
Copy link

I'm not sure how widespread S7 is going to be adopted in either BioC or tidyverse, but if it isn't, the mechanism of delayed registration for external generics might be adopted/copied (S7 is MIT licenced). It might make most sense to have the mechanism live in BiocGenerics, on which S4Vectors already depends, and then used here. Possibly, it might be useful to untangle a wider web of very shallow dependencies.

@hpages
Copy link
Contributor

hpages commented Sep 20, 2023

This just got merged by @teunbrand a few minutes ago (thanks Teun!), making this PR unnecessary.

Thanks @antagomir and @kevinrue for starting the discussion on the BioC Community Slack and helping with the process.

@hpages hpages closed this Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants