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

Projection drive r #38

Merged
merged 35 commits into from
Feb 21, 2024
Merged

Projection drive r #38

merged 35 commits into from
Feb 21, 2024

Conversation

dimalvovs
Copy link
Collaborator

@rpalaganas adding a pull request for (my) review here

Ryan Palaganas and others added 22 commits November 3, 2023 16:59
Added tests for projectionDriveR in test_projectR.R
Removed redundant confidence interval (CI) plotting function from projectionDriveRfun.R
Updated CI plotting function in plotting.R to include weighted vs unweighted CI plots
Removed redundant projectionDriveR function
Updated projectionDriveR function with mode argument, user specifies generation of confidence intervals or pvalues

Updated plotting.R with volcano plotting function

Added tests for updated functions
Updated vignette to reflect updated projectionDriveR function
include magick package
include ggpubr import
install specific branch
Removed rmd file to check if it is causing build errors
update projectR.html
Removed cropping and magick from vignette
Removed plot print
@dimalvovs dimalvovs self-assigned this Jan 19, 2024
@dimalvovs
Copy link
Collaborator Author

dimalvovs commented Jan 19, 2024

@rpalaganas thanks for a lot of comments, really helpful to understand what is happening. Also I think the variable names are well thought of, although some of them could have been shorter.

  • Overall, I did not grasp the logical separation between functionality in plotting.R and projectionDriveRfun.R, especially in the light of projectionDriveRfun.R also doing plotting. Is all plotting in package being done through plotting? If yes, maybe projectionDriveRfun.R plotting should be moved there. If no, maybe plotting.R needs a name update.

projectionDriveRfun.R
The overall length of function code is quite large compared to arbitrary 50 line advisedlength, consider shortening. for example,

  • there is no need to say warning(paste0('a', 'b')) as warning(a, b) works just in the same way.
  • consider message instead of print, it works just in the same way as warning(), i.e. no need for paste()
  • consider style: remove trailing spaces and some of line returns like lines 18-20, use <- instead of = etc,
  • lines 107-124 can most probably written 2-3 lines e.g. using something likecbind and as.data.frame
  • consider splitting CI and PV modes into separate funs
  • stop does not require paste

plotting.R

  • what are head rows up to line 11 meant for?
  • same as previous function regards style, consider adding a linter, example
  • same as above consider modularizing the and shrinking the lengthy functions, apart from more concise code maybe unweightedvolcano and weightedvolcano can become separate funs, or even there could be one parametrized fun for both?

dimalvovs and others added 6 commits January 19, 2024 16:49
Improved style consistency, incorporated lintr
Parameterized unweighted/weighted volcanos to one function
Updated vignette with more projectionDriveR examples
updated roxygen2 documentation
Incorporated some lintr suggestions
Resolved 'no visible binding for global variable'
Added automatic plotting
Condensed volcano plotting to one parametrized function
Incorporated some lintr suggestions
Added display options for volcano plots
Added examples for PV mode projectionDrivers
Added fgsea examples for pdVolcano function
Knit rmd using projectiondriver branch. Should be rerun after merge
Removed unnecessary == FALSE/TRUE statements
@dimalvovs dimalvovs merged commit 83da394 into master Feb 21, 2024
3 checks passed
@dimalvovs dimalvovs deleted the projectionDriveR branch February 21, 2024 20:08
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.

2 participants