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

updated priors vignette so that it works, exported required functions #2439

Merged
merged 18 commits into from Nov 1, 2019

Conversation

dlebauer
Copy link
Member

Updated the prior vignette so that it can compile. This required exporting a few functions from the PEcAn.priors function. Or perhaps these were incorrectly identified as S3 methods.

Motivation and Context

To get the vignette working again.

Review Time Estimate

  • Immediately
  • Within one week
  • When possible

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the CHANGELOG.md.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

CHANGELOG.md Outdated Show resolved Hide resolved
Co-Authored-By: Chris Black <chris@ckblack.org>
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.

@dlebauer Overall this looks great. Thanks for getting it working again!

To get Travis tests passing, I think it should suffice to add MASS to the Imports section of the DESCRIPTION file. (Note that the check failure is only detecting the undeclared import, not running the vignette code -- making Travis actually build vignettes is a separate issue discussed in #2437).

Once tests pass, I'm happy to merge this, but a longer-term consideration that can wait for a different PR: This vignette uses a lot of external packages that aren't currently listed as dependencies for the Priors package -- I see at least dplyr, DEoptim, fitdistrplus, rjags, rriskDistributions. These need to be declared as package dependencies, so we should (1) consider whether they add enough to the vignette to be worth the installation footprint or whether we can show usage effectively without them, then (2) remove any we can live without and (3) declare the rest in Suggests.

@dlebauer
Copy link
Member Author

dlebauer commented Oct 3, 2019 via email

@infotroph
Copy link
Member

my version of Roxygen wants to re-add these S3 method declarations. How did you persuade it to treat these as regular functions?

No idea! I think I just used devtools::document()

@dlebauer Huh, weird. Well, I'd say fix the missing MASS declaration and see if the build passes -- if it does, the problem was on my end. If it complains about files changing during the build, then we'll need to revisit this

@infotroph
Copy link
Member

@dlebauer Can you estimate when you'll have time to finish this? I'd like to use the priors package as a test case for #2437, but it'd be preferable to have this merged first to avoid conflict / take advantage of your work

@dlebauer
Copy link
Member Author

@infotroph I've added MASS to imports

regarding the dependencies ... I think fitdistrplus, rjags, and rriskDistributions are critical to the vignette - since the vignette shows how to use these packages to come up with priors. DEoptim could possibly be replaced with optim and dplyr could certainly be replaced. But then this brings up the question of whether it is worthwhile to refactor code just to reduce dependencies (especially in vignettes, or in the case of dplyr, where the package is installed elsewhere in the PEcAn build).

modules/priors/R/plots.R Outdated Show resolved Hide resolved
modules/priors/R/plots.R Outdated Show resolved Hide resolved
@infotroph infotroph mentioned this pull request Nov 1, 2019
* remove unneeded Roxygen tags, keep documentation aliases to old names

* change references elsewhere

* remove s3 inconsistency warning from cached check results

* fix easy check errors in renamed functions, add harder ones to cached checks list

* changelog

* update depends
@dlebauer
Copy link
Member Author

dlebauer commented Nov 1, 2019

Thanks for your help @infotroph! Finally can merge!

@dlebauer dlebauer merged commit 4dfd00d into develop Nov 1, 2019
@dlebauer dlebauer deleted the prior_vignette branch November 1, 2019 11:13
@robkooper robkooper added this to the 1.8.0 milestone Apr 14, 2020
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

4 participants