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

Optimization and test infrastructure #12

Closed
wants to merge 45 commits into from

Conversation

doug-friedman
Copy link

I know this is a big pull request, but everything has been documented carefully below. Feel free to edit as makes sense.

  • Add formal unit testing with testthat
  • Move documentation over to roxygen
  • Transition dependencies to imports
  • Transition data to data-raw and use rda data
  • Speed up the L-moments calculations (where possible)
  • Make general performance improvements
  • Remove pglo.R and update documentation
  • Change default plot title to include "SPI"/"SPEI"" when appropriate
  • Change default y-axis title to say "z-values"
  • Allow user to change the title when plotting spi/spei
  • Increase test coverage to 75%
  • Increase test coverage to 90%

@sbegueria
Copy link
Owner

Hi Doug, many thanks for your extensive PR. I have just updated a new version of the library that includes some of your mods (those related to the plotting function). I will now work on merging your fork.

@doug-friedman
Copy link
Author

Great. Thanks!

If you've got any questions or concerns, don't hesitate to ask.

@sbegueria
Copy link
Owner

sbegueria commented Jan 4, 2018

Hi Doug, I could finally find some time to work on your pull request. Since there were too many conflicts with my latest master version I did the merge by hand. Your changes are in the main branch now.

I'm working now on the tests, since some of them are failing due to my last changes to the function kern that motivated the change to version 1.7 of the package.

... Still struggling with the tests. I get no errors when test_dir('tests/'), but then devtools::check() gives me one error:

══ testthat results
OK: 36 SKIPPED: 0 FAILED: 1

  1. Failure: summary of spei object (@test_spei.R#45)

@doug-friedman
Copy link
Author

I'm not quite sure why it failed, but likely my inclusion of capture.output was the culprit. If it's creating an issue, feel free to just comment it out.

@sbegueria
Copy link
Owner

I close this now as all the suggested commits (or most part of them) have been included or have been superseded in the development version of the package.

@sbegueria sbegueria closed this May 4, 2022
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