Skip to content

bugfix for high frequency time series in scaling.py, regarding issue … #1258

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

Merged
merged 2 commits into from
Aug 8, 2021

Conversation

jranalli
Copy link
Contributor

@jranalli jranalli commented Jul 11, 2021

…1257

  • [ X ] Closes Error in scaling.py when sampling rate faster than 1s. #1257
  • [ X ] I am familiar with the contributing guidelines
  • [ X ] Tests added
  • [ X ] Updates entries to docs/sphinx/source/api.rst for API changes.
  • [ X ] Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
  • [ X ] New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • [ X ] Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.

When passing pvlib.scaling._compute_wavelet a pandas timeseries with a time resolution faster than 1s, the use of .seconds to calculate the time series delta_t produced an overflow error. It occurs because .seconds only computes integer values of seconds. I added a computation including microseconds to account for fractional seconds in this case.

@jranalli
Copy link
Contributor Author

I'm going to push an additional commit proposing a slight refactor to scaling.wvm. I have a use case for having a separate computation of the variability reduction factors used by the WVM. In a recent conference paper I was looking at how to apply the WVM in reverse to increase the spatial resolution. While I'm not proposing we include that reverse case (until such time as someone is able to generate more comprehensive published results for it), having a computation of the VR available would make it possible to do that calculation on your own with minimal re-writing of WVM code to perform those calculations. It has no impact on the existing WVM calculation, but I did write a test for the proposed new function.

If putting this change here is inappropriate, or this is an unwanted change, we can just unwind this commit and stick with the bugfix above.

@cwhanse
Copy link
Member

cwhanse commented Jul 12, 2021

The refactor makes the main function more readable, IMO. I'd keep it and if it supports future capability, all the better.

@jranalli
Copy link
Contributor Author

Sounds good, I will fix the stickler errors and squash the commits then.

@wholmgren wholmgren mentioned this pull request Jul 30, 2021
24 tasks
@cwhanse cwhanse added this to the 0.9.0 milestone Aug 2, 2021
@cwhanse cwhanse added the bug label Aug 2, 2021
Copy link
Member

@cwhanse cwhanse left a comment

Choose a reason for hiding this comment

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

@jranalli can you remove the extra line in the whatsnew file?

stickler issues

Refactor to separate out VR computation within pvlib.scaling.wvm to a new function pvlib.scaling._compute_vr.

bugfix for high frequency time series in scaling.py, regarding issue 1257

add pull request number

bugfix for high frequency time series in scaling.py, regarding issue 1257
@jranalli
Copy link
Contributor Author

jranalli commented Aug 2, 2021

Sorry about that, I think I got it.

@wholmgren
Copy link
Member

@cwhanse can you merge if this is ready?

@cwhanse
Copy link
Member

cwhanse commented Aug 8, 2021

thanks @jranalli

@cwhanse cwhanse merged commit 9543d87 into pvlib:master Aug 8, 2021
@jranalli
Copy link
Contributor Author

jranalli commented Aug 8, 2021

No problem, thanks for including it!

@jranalli jranalli deleted the scaling_dt_fix branch May 26, 2022 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error in scaling.py when sampling rate faster than 1s.
3 participants