method documentation & vignette revision #74
Conversation
There was a problem hiding this comment.
Thanks @HelenaLC !
- I notice that we have repetitions (that's an old problem, this is likely true for all methods) such as
?bind_rows
Description
This is an efficient implementation of the common pattern of 'do.call(rbind, dfs)' or 'do.call(cbind, dfs)' for binding many data frames into one.
This is an efficient implementation of the common pattern of 'do.call(rbind, dfs)' or 'do.call(cbind, dfs)' for binding many data frames into one.
Details
The output of 'bind_rows()' will contain a column if that column appears in any of the inputs.
The output of 'bind_rows()' will contain a column if that column appears in any of the inputs.
Value
'bind_rows()' and 'bind_cols()' return the same type as the first input, either a data frame, 'tbl_df', or 'grouped_df'.
'bind_rows()' and 'bind_cols()' return the same type as the first input, either a data frame, 'tbl_df', or 'grouped_df'.-
For the bind_rows example, I would prefer to avoid
ttservice::, or replace it withtidySingleCellExperiment::, as users should not know about ttservice. -
One thing I was trying to clean is to drop all
`%>%` <- magrittr::`%>%`From the documentation and use the base pipe |> everywhere
- I noticed that if I do
?slicethe documentation includes all methods, e.g.
`%>%` <- magrittr::`%>%`
pbmc_small %>%
arrange(nFeature_RNA)
`%>%` <- magrittr::`%>%`
pbmc_small %>%
distinct(groups)
`%>%` <- magrittr::`%>%`
pbmc_small %>%
filter(groups == "g1")
# Learn more in ?dplyr_tidy_eval
`%>%` <- magrittr::`%>%`
pbmc_small %>%
group_by(groups)
`%>%` <- magrittr::`%>%`
pbmc_small %>%
summarise(mean(nCount_RNA))
this is also true for ?select and I assume for other methods as well.
I took those files from the tidyverse package. Would add them to depend load those package analogously to what attach.R does? Maybe changing this could be another pull request. I am never against simplifying stuff ;)
Yes I'm not sure why If only |
Amazing! |
|
For 1. I think this is could be due to how ttservice documents them, but will doublecheck. I.e., we get what we inherit, so fixing it upstream might resolve it. 2.-4. Agreed & I can fix those. Re bind_rows etc., still worth thinking about how we can resolve this. Eg, as a user, I get pretty confused with non-informative errors because some other method was dispatched :/ |
Let's think about this after this PR then.
When these are done, most likely the PR is good to go! Looking forward to accept it. (also please have a look if they pass the github actions, before requiring code review) |
|
Hm. I wonder if these fails could be related to older R/Bioc versions in the GHA? I could not reproduce them on R 4.3/Bioc 3.17, so am a bit stuck how to fix this. |
I see just one warning must be some little bug, I don't think it depends on versions of R/Bioc |
Yeah, I think you're right - hoping this'll fix it! Maybe worth considering to eventually up GHA versions in any case, just to be sure we're testing in the same realm as Bioc servers will (as to not get unexpected build/check warnings/errors when pushing upstream). |
To have quicker iterations you could even PR onto your master, test, and then push your master into mine after it passes the tests. The warning is because it thinks, for example, we are extending |
|
Replaced by #75 |
tidySE,tidySCE,tidySPE,tidyseurattidyseurat#66, includingBiocStyle::CRAN/Biocpkg()and friendsecosystems (tidyverse), classes/variables (
tibble), functions (unnest())tidySingleCellExperiment's functionalitytidySE,tidySCE,tidySPE,tidyseurattidyseurat#66, includingdplyr,tidyr, ..., methodsattach.Randzzz.Rfrom drop exports #69 to assure needed dependencies are attachedattach.Randzzz.R, would depending (instead of importing) packages be an option? how to other tidy packages deal with defining for S3 methods for foreign generics?dplyr'sbind_rows/cols()masks thettserviceversion; currentlyneed to write
ttservice::bind_rows/cols()for methods to workdevtools::document()is warning-free