haowulab / DSS

14 stars 13 forks source link

Subclass `"data.frame"` rather than superclassing it #32

Closed DavisVaughan closed 1 year ago

DavisVaughan commented 1 year ago

We got a report in https://github.com/r-lib/vctrs/issues/1758 that the DMLtest data frame class isn't compatible with dplyr operations. It looks like you superclass data frame with the class being c("data.frame", "DMLtest"). We don't allow this in vctrs/dplyr, which needs you to subclass a data frame for it to be compatible with dplyr, like c("DMLtest", "data.frame"). I'm hoping that difference isn't very important for you.

I've also fixed some usage of class() == in if statements, which is considered bad practice over using inherits() (R CMD check will warn about this).

@llrs hopefully this fixes your issue.

llrs commented 1 year ago

Many thanks @DavisVaughan for finding the source in Github.

I hope this PR will be merged to make it easier manipulate the data with dplyr and other related packages.

haowulab commented 1 year ago

@DavisVaughan, how can I merge this change to bioconductor? I'm trying to push to upstream master but can't. Honestly, I'm confused about the bioconductor versions, especially it just have RELEASE_3_16. Could you please help? Thank you very much.

haowulab commented 1 year ago

Okay it seems I did it. I committed to upstream master. Is this what I supposed to do? Or I should commit to 3.16?

llrs commented 1 year ago

Hi @haowulab, you can check the Bioconductor guides about how to manage the different repositories:

You currently merged it into the GitHub repository (with commit 7b6a3c4) but to check if this changes are pushed to version 3.16 of Bioconductor you can check this page. Currently it doesn't show up the changes. But as this is not a bug fix (imho) you can push to the master branch after pulling the changes. This depends on how you have set up your computer but something like this might work:

git pull upstream
git pull
git push