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

Fix remaining check warnings and undeclared dependencies from PEcAn.utils #2830

Merged
merged 94 commits into from Aug 31, 2021

Conversation

moki1202
Copy link
Collaborator

No description provided.

@moki1202 moki1202 marked this pull request as draft July 30, 2021 10:38
@moki1202
Copy link
Collaborator Author

/document

Copy link
Member

@infotroph infotroph left a comment

Choose a reason for hiding this comment

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

Incomplete review, will try to finish tomorrow, but I am PUMPED to see these deprecated functions going away! Let's make sure we record those in the changelog in this PR and close their tracking issues (I'll find and link in comments) once this is merged

base/utils/R/utils.R Outdated Show resolved Hide resolved
base/utils/man/n.Rd Outdated Show resolved Hide resolved
base/utils/R/sensitivity.R Outdated Show resolved Hide resolved
base/utils/R/utils.R Outdated Show resolved Hide resolved
base/utils/DESCRIPTION Outdated Show resolved Hide resolved
@moki1202
Copy link
Collaborator Author

@infotroph there are still 1-2 fixes remaining for utils but I think we can do that in another PR.

@infotroph
Copy link
Member

@moki1202 I agree. Any remaining changes in this PR will be ones that are requested by reviewers other than me or that are needed to make the CI build pass, so I'll mark it "ready for review" to let other folks know they can start reading it.

@infotroph infotroph marked this pull request as ready for review August 30, 2021 09:12
To fix correctly, load samples.Rdata into its own environment
and access variables from there rather than load into top-level env
@@ -56,6 +56,7 @@
##' @param ensemble An integer representing the number of ensembles, or FALSE if it data product is not an ensemble.
##' @param ensemble_name If convert.input is being called iteratively for each ensemble, ensemble_name contains the identifying name/number for that ensemble.
##' @param ... Additional arguments, passed unchanged to \code{fcn}
##' @param dbparms list of parameters to use for opening a database connection
Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend putting the args in the same order in the documentation as they are in the function, and definitely with ... last. Also, seems like we should say what goes in the dbparms list (or at least crossreference someplace that does)

##' @return a filename
##' @export
##'
##' @details Generally uses values in settings, but can be overwritten for manual uses
##' @author Ryan Kelly
sensitivity.filename <- function(settings,
prefix = "sensitivity.samples", suffix = "Rdata",
sensitivity.filename <- function(settings,
Copy link
Member

Choose a reason for hiding this comment

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

maybe I'm misremembering, but weren't you moving the sensitivity functions to the uncertainty package?

Copy link
Member

Choose a reason for hiding this comment

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

We are but we missed this file. @moki1202 I think this will be resolved when you merge moki1202#5 (assuming you agree with my fix!)

##' @param per.pft flag to determine whether we want SA on pft-specific variables
##' @param sa.run.ids list of run ids to read.
##' If NULL, will look in `pecandir` for a file named `samples.Rdata`
##' and read from that
Copy link
Member

Choose a reason for hiding this comment

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

same question as above about where sensitivity functions should live

Makefile.depends Outdated Show resolved Hide resolved
@infotroph
Copy link
Member

Whew, all checks passing!

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.

None yet

3 participants