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

Improve documentation and package imports #308

Draft
wants to merge 40 commits into
base: master
Choose a base branch
from

Conversation

william-hutchison
Copy link

This pull request picks up on @chilampoon's pull request #297 . In summary:

  • Polish roxygen2 function documentation (e.g inheriting from other packages when possible)
  • Improve coding style (e.g "=" converted to "<-")
  • Switch Github actions workflow from check-bioc to rworkflows
  • Document function imports in roxygen2, NAMESPACE and DESCRIPTION instead of installing and loading packages within function calls
  • Update tidyomics ecosystem description in the README and vignette
  • Replace the example data used in the README

Copy link
Owner

@stemangiola stemangiola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

Comment on lines +41 to +64
AnnotationDbi,
DESeq2,
Rtsne,
Seurat,
betareg,
boot,
broom,
class,
clusterProfiler,
e1071,
edgeR,
functional,
glmmSeq,
glmmTMB,
limma,
lme4,
matrixStats,
msigdbr,
org.Hs.eg.db,
org.Mm.eg.db,
pbapply,
pbmcapply,
survival,
survminer,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking this through!

I really think the dependency structure should go back as before. Please try to remove dependency gradually and see if error appear.

Is there any reason to add tidySummarizedExperiment in Suggests?

*please open a new branch for this

Comment on lines -258 to 261
# Check if package is installed, otherwise install
if (find.package("matrixStats", quiet = TRUE) %>% length %>% equals(0)) {
message("tidybulk says: Installing matrixStats needed for cibersort")
install.packages("matrixStats", repos = "https://cloud.r-project.org")
}

# Eliminate sd == 0
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reinstate this would also be part of the effort of dependencies.

@chilampoon
Copy link

Thanks @chilampoon! Yep sounds like a plan, see how you go and I will attempt to fix whatever remains.

hi @william-hutchison can you add me as a contributor to your tidybulk repo? thx

@stemangiola
Copy link
Owner

Thanks @chilampoon! Yep sounds like a plan, see how you go and I will attempt to fix whatever remains.

hi @william-hutchison can you add me as a contributor to your tidybulk repo? thx

Hello @chilampoon we interact with forks for the tidyomics. Is much better to track who has contributed to what and see the repo history.

@chilampoon
Copy link

Thanks @chilampoon! Yep sounds like a plan, see how you go and I will attempt to fix whatever remains.

hi @william-hutchison can you add me as a contributor to your tidybulk repo? thx

Hello @chilampoon we interact with forks for the tidyomics. Is much better to track who has contributed to what and see the repo history.

I meant his forked tidybulk repo so I can directly add commits to this branch

@william-hutchison
Copy link
Author

@chilampoon, sounds good. I have added you as a collaborator to my fork.

@stemangiola
Copy link
Owner

@william-hutchison please update all forks from my master (I have solved the ERROR in the tests).

@stemangiola
Copy link
Owner

@william-hutchison please update all forks from my master (I have solved the ERROR in the tests).

Pleaser @chilampoon pull again from my master. I have fixed a bug.

@chilampoon
Copy link

chilampoon commented Feb 12, 2024

@william-hutchison please update all forks from my master (I have solved the ERROR in the tests).

Pleaser @chilampoon pull again from my master. I have fixed a bug.

it's in this commit Merge remote-tracking branch 'stemangiola/master' into ...

@stemangiola
Copy link
Owner

@william-hutchison please update all forks from my master (I have solved the ERROR in the tests).

Pleaser @chilampoon pull again from my master. I have fixed a bug.

it's in this commit Merge remote-tracking branch 'stemangiola/master' into ...

Needs to be done again :)

Comment on lines +41 to +64
AnnotationDbi,
DESeq2,
Rtsne,
Seurat,
betareg,
boot,
broom,
class,
clusterProfiler,
e1071,
edgeR,
functional,
glmmSeq,
glmmTMB,
limma,
lme4,
matrixStats,
msigdbr,
org.Hs.eg.db,
org.Mm.eg.db,
pbapply,
pbmcapply,
survival,
survminer,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chilampoon and @william-hutchison we need to drop all these new Imports. If Bioconductor does not accept on-the-fly installation anymore, we rather print the message to install if the packages are not installed, and then use

<package>::<function>

but without adding those dependendies to @importFrom, not in DESCRIPTION as Imports

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yead I agree on too many Imports, however directly add <package>::<function> inside a function without specifying `@importFrom' will trigger a warning saying '::' or ':::' imports not declared from: xyz packages...

#' @importFrom scales extended_breaks
#' @importFrom stats qlogis plogis
#' @importFrom functional Compose
#'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would not add any dependency here, but check if functional is installed, and if not , error the user, asking for installing.

This should be applied to all cases.

Comment on lines +425 to 432
#'
#' @import lme4
#' @importFrom glmmTMB glmmTMBControl
#' @importFrom parallel makeCluster
#' @importFrom parallel clusterExport
#' @importFrom pbapply pblapply
#' @importFrom pbmcapply pbmclapply
#'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Comment on lines +1113 to 1119
#' @importFrom betareg betareg
#' @importFrom broom tidy
#' @importFrom boot logit
#' @importFrom survival coxph
#' @importFrom survminer surv_fit
#' @importFrom survminer ggsurvplot
multivariable_differential_tissue_composition = function(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

@stemangiola
Copy link
Owner

stemangiola commented Mar 6, 2024

Hello @chilampoon, I hope you're well. Any news with this project? We have put a lot of energies, it would be worth to take it to completion.

@chilampoon
Copy link

chilampoon commented Mar 10, 2024

Hello @chilampoon, I hope you're well. Any news with this project? We have put a lot of energies, it would be worth to take it to completion.

yes lets get it done. Re the package dependencies - so many functionalities are included in this package, I wonder if it's possible to remove some of the non-core functions, especially those for running very downstream tasks? instead we could just keep some examples in the vignette, then the package would be more lightweight

@stemangiola
Copy link
Owner

stemangiola commented Mar 10, 2024

Hello @chilampoon, I hope you're well. Any news with this project? We have put a lot of energies, it would be worth to take it to completion.

yes lets get it done. Re the package dependencies - so many functionalities are included in this package, I wonder if it's possible to remove some of the non-core functions, especially those for running very downstream tasks? instead we could just keep some examples in the vignette, then the package would be more lightweight

Sure we can have this discussion. However better we do one thing at the time.

@chilampoon I see the problem here i that the Github do not pass. I think we are using a not efficient way to approach this problem. Are you doing devtools::check() locally before pushing to the GiHub?

We might be close, but I feel we are stuck on this loop. Do you feel you would like a brief meeting on the best testing checking strategy?

or @william-hutchison do you feel it would be easier for you to bring this PR to completion?

@chilampoon
Copy link

Yes I run devtools::check() and R CMD CHECK before committing, there are still errors/warnings I haven't addressed.
Sure I am available to meet on Thursday & Friday, my time is EDT @stemangiola

@stemangiola
Copy link
Owner

Hello @chilampoon, sorry for the delay in grant writing time. Any chance we could solve this offline? @william-hutchison, maybe you could advise on why checks are still failing.

@chilampoon
Copy link

chilampoon commented May 16, 2024

Hello @chilampoon, sorry for the delay in grant writing time. Any chance we could solve this offline? @william-hutchison, maybe you could advise on why checks are still failing.

Hi yeah I have been preparing for my candidacy exam which will happen in two weeks, maybe @william-hutchison could help point out the issues by then? And I can meet on zoom in the second week of June

@stemangiola
Copy link
Owner

stemangiola commented Aug 26, 2024

@arashbioinfo decided to try to solve this PR. @william-hutchison can give you an hand if you get stuck.

@arashbioinfo, the first thing is to solve the "conflicts", there are plenty of tutorial on how to deal with them. Conflict emerge when the master branch diverges a lot from the pool request.

Maybe @william-hutchison , could you solve the conflicts since you have more background on the package, then @arashbioinfo will try to solve the github checks

@arashbioinfo
Copy link

@arashbioinfo decided to try to solve this PR. @william-hutchison can give you an hand if you get stuck.

@arashbioinfo, the first thing is to solve the "conflicts", there are plenty of tutorial on how to deal with them. Conflict emerge when the master branch diverges a lot from the pool request.

Maybe @william-hutchison , could you solve the conflicts since you have more background on the package, then @arashbioinfo will try to solve the github checks

Thank you, @stemangiola, for the guidance and support. I appreciate the opportunity to contribute to this PR.

@william-hutchison, thank you in advance for your help. If possible, could we arrange a brief meeting to discuss the best approach to resolving this PR and aligning our efforts?

@william-hutchison
Copy link
Author

Hi @arashbioinfo, sure. Would you like to email me at [email protected] to arrange a time? I'm looking forward to working with you on this!

@william-hutchison william-hutchison marked this pull request as draft September 3, 2024 03:51
@william-hutchison
Copy link
Author

Hi @arashbioinfo. I have resolved the conflicts between this PR and the tidybulk master branch. The next step in this project will be to work out why some tests are now failing and correct the code which is responsible. Good luck!

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