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

fixed some global variables in PEcAn.MA package #2965

Merged
merged 39 commits into from
Jul 20, 2022

Conversation

nanu1605
Copy link
Collaborator

No description provided.

nanu1605 and others added 24 commits July 12, 2022 09:21
Merge branch 'meta_analysis' of https://github.com/nanu1605/pecan into meta_analysis
@nanu1605 nanu1605 requested review from infotroph and Aariq July 17, 2022 17:13
@infotroph
Copy link
Member

infotroph commented Jul 18, 2022

It looks like this branch contains a lot of changes (everything up to commit 8ad821e) that are already present in #2956 -- this will cause issues when trying to merge them all into develop. @nanu1605 can you please cherry-pick the last three commits (95880de, 49905f5, 57c8786) into a new branch that starts from the current tip of the develop branch, open a new PR from that branch, and close this one?

I'm sorry that this will be some hassle, but it's better to resolve it now than to wait -- Git branch divergences only get more painful the longer they continue.

Edit: I take it back. Since this branch contains ALL the changes from #2956 and #2963, we can resolve this cleanly by merging this PR (once it passes CI) and closing the other two.

modules/meta.analysis/R/run.meta.analysis.R Show resolved Hide resolved
modules/meta.analysis/R/run.meta.analysis.R Outdated Show resolved Hide resolved
modules/meta.analysis/R/run.meta.analysis.R Outdated Show resolved Hide resolved
@nanu1605 nanu1605 requested a review from dlebauer July 18, 2022 21:02
Copy link
Member

@dlebauer dlebauer left a comment

Choose a reason for hiding this comment

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

looks great!

  • most important thing would be to add a comment about how and why new.env is being used
  • consider not exporting the p.point.in.prior function

modules/meta.analysis/R/run.meta.analysis.R Show resolved Hide resolved
modules/meta.analysis/R/run.meta.analysis.R Outdated Show resolved Hide resolved
modules/meta.analysis/R/run.meta.analysis.R Outdated Show resolved Hide resolved
modules/meta.analysis/R/run.meta.analysis.R Show resolved Hide resolved
modules/meta.analysis/R/run.meta.analysis.R Outdated Show resolved Hide resolved
@@ -235,15 +242,12 @@ runModule.run.meta.analysis <- function(settings) {
##'
##' used to compare data to prior, meta analysis posterior to prior
##' @title find quantile of point within prior distribution
##' @param point
##' @param point quantile of given prior to return
##' @param prior list of distn, parama, paramb
##' @return result of p<distn>(point, parama, paramb)
##' @export p.point.in.prior
Copy link
Member

@dlebauer dlebauer Jul 18, 2022

Choose a reason for hiding this comment

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

Suggested change
##' @export p.point.in.prior

I think this will at least make it an unexported function, which is appropriate since it isn't particularly useful outside of the one place in run.meta.analysis where it is used:

p.data <- p.point.in.prior(point = data.median, prior = prior)

FYI if you replaced this line in run.meta.analysis with

  p.data <- do.call(paste0("p", prior[['distn']]), 
                              list(point, prior[['parama']], prior[['paramb']]))

and deleted the test of this function I think that would be fine. This isn't necessary, but something to keep an eye out for useless functions that are my legacy from PEcAn v 0.1, and that could make everyone's life easier by not exporting, and perhaps deprecating and merging with existing code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure I will do the changes and commit ASAP.

modules/meta.analysis/R/single.MA.R Show resolved Hide resolved
@mdietze
Copy link
Member

mdietze commented Jul 20, 2022

@dlebauer just wanted to check if @nanu1605 addressed your requested changes, as otherwise I think this is ready to pull in.

@dlebauer dlebauer merged commit fbfcd2f into PecanProject:develop Jul 20, 2022
@nanu1605 nanu1605 deleted the meta_analysis_global_variables branch July 20, 2022 16:40
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.

5 participants