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

Update the time shift Ruptures function with the new logic that was tested/validated at PVRW 2023 #197

Merged
merged 14 commits into from Dec 22, 2023

Conversation

kperrynrel
Copy link
Collaborator

@kperrynrel kperrynrel commented Aug 9, 2023

This update makes the changes to the original function so it now matches the finalized logic that I used for validation in the PVRW 2023 poster (https://www.nrel.gov/docs/fy23osti/85699.pdf). Most notably, we use binary segmentation instead of Pelt here, which results in a massive speedup while still being performant.

  • Added tests to cover all new or modified code.
  • Clearly documented all new API functions with PEP257 and numpydoc compliant docstrings.
  • Added new API functions to docs/api.rst.
  • Non-API functions clearly documented with docstrings or comments as necessary.
  • Adds description and name entries in the appropriate "what's new" file
    in docs/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`).
  • 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.

@kperrynrel kperrynrel marked this pull request as draft August 9, 2023 18:09
@kperrynrel kperrynrel added this to the v0.2.0 milestone Aug 9, 2023
@kperrynrel kperrynrel added the enhancement New feature or request label Aug 9, 2023
@kandersolar kandersolar marked this pull request as ready for review December 13, 2023 17:31
Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me! A couple minor comments.

pvanalytics/quality/time.py Outdated Show resolved Hide resolved
changepoint detection. All data below this threshold is not considered
when determining the mean value for the segment, which is later
rounded to the nearest `period_min` value
top_quantile_threshold : float, default 0.5
Copy link
Member

Choose a reason for hiding this comment

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

Just curious... these default values exclude the top 50% of values within each segment, right? I'm surprised that such an symmetrical window is a good default choice. What's the reasoning for it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kandersolar this was experimentally derived, and meant to filter out noise. The original code did a 2/3rd cutoff to make the code more conservative about time shifts (we only want to suggest a shift if we're absolutely certain, and we want to error on the side of caution). Essentially this parameter allows the user to decide how cautious they want to be when estimating time shifts.

@kandersolar kandersolar merged commit e7aeb91 into pvlib:main Dec 22, 2023
35 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants