Skip to content

Multi yoy integration - combines PRs #456, #460 and #464 at once.#470

Open
cdeline wants to merge 86 commits intodevelopmentfrom
multi_YoY_integration
Open

Multi yoy integration - combines PRs #456, #460 and #464 at once.#470
cdeline wants to merge 86 commits intodevelopmentfrom
multi_YoY_integration

Conversation

@cdeline
Copy link
Collaborator

@cdeline cdeline commented Dec 10, 2025

See individual PR's #456, #460 and #464 for discussion.

Issues & PRs

  • Code changes are covered by tests
  • Code changes have been evaluated for compatibility/integration with TrendAnalysis
  • New functions added to __init__.py
  • API.rst is up to date, along with other sphinx docs pages
  • Example notebooks are rerun and differences in results scrutinized
  • Updated changelog

cdeline and others added 30 commits May 28, 2025 14:31
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR merges prior work to support multi-Year-on-Year (multi-YoY) degradation analysis and improved time-labeling options across calculations and plots, plus accompanying tests and documentation/CI updates.

Changes:

  • Add label and multi_yoy options to degradation_year_on_year, including new calc metadata and updated point-usage handling.
  • Add label option to degradation_timeseries_plot and extend plotting tests (including a multi-YoY plotting scenario).
  • Update docs/changelog and notebook/nbval workflow configuration for notebook testing.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
setup.py Unpins nbval in test requirements.
rdtools/degradation.py Adds label + multi_yoy support and related calc-info changes.
rdtools/plotting.py Adds label support to the degradation time-series plot and updates summary plot coloring behavior.
rdtools/test/degradation_test.py Adds tests for label='center', _avg_timestamp_old_Pandas, and multi_yoy=True.
rdtools/test/plotting_test.py Extends degradation time-series plotting tests to cover labeling and multi-YoY behavior.
docs/system_availability_example.ipynb Notebook output/display tweak.
docs/sphinx/source/changelog/pending.rst Adds pending changelog entries for new parameters/features.
docs/sphinx/source/changelog.rst Includes the pending changelog into the main changelog.
docs/TrendAnalysis_example_NSRDB.ipynb Minor notebook code formatting tweak.
docs/TrendAnalysis_example.ipynb Minor notebook code formatting tweaks.
.github/workflows/nbval.yaml Updates nbval sanitize flag usage and command invocation.
Comments suppressed due to low confidence (1)

rdtools/degradation.py:237

  • The usage_of_points docstring describes only values 0/1/2, but multi_yoy=True can produce larger counts. Please update the docstring to reflect that counts may exceed 2 in multi-YoY mode (and what higher values mean).
          probability of `exceedance_prob`
        * `usage_of_points` - number of times each point in energy_normalized
          is used to calculate a degradation slope. 0: point is never used. 1:
          point is either used as a start or endpoint. 2: point is used as both
          start and endpoint for an Rd calculation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 502 to 509
center = False
offset_days = 0
elif label == "center":
center = True
offset_days = 182
elif label == "left":
center = True
offset_days = 365
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

For label='center', the code sets center=True on the rolling window and shifts the plotted x-values by offset_days=182. Because center=True already shifts the result index to the window midpoint, subtracting an additional 182 days effectively double-shifts the timestamps and is inconsistent with the docstring’s “center of rolling median interval”. Consider either (a) not shifting when center=True, or (b) using center=False and shifting by half the window, but not both.

Suggested change
center = False
offset_days = 0
elif label == "center":
center = True
offset_days = 182
elif label == "left":
center = True
offset_days = 365
# Right-labeled: rolling index at right edge, no offset
center = False
offset_days = 0
elif label == "center":
# Center-labeled: rolling index at midpoint, no additional offset
center = True
offset_days = 0
elif label == "left":
# Left-labeled: rolling index at right edge, shift by full window to left edge
center = False
offset_days = rolling_days

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@cdeline - If you get a chance, could you take a look at this suggestion?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@martin-springer - yeah I'm going to need to change this logic because now the YoY-Info dict being passed in is no longer explicitly right-labeled. I'm going to revert it back to just be center=True or center=False. We'll need to update the TA object as well to allow it to have correct label options.

if label not in {None, "right", "left", "center"}:
raise ValueError(f"Unsupported value {label} for `label`."
" Must be 'right', 'left' or 'center'.")
if label is None:
Copy link
Collaborator

@martin-springer martin-springer Feb 12, 2026

Choose a reason for hiding this comment

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

users would need to pass label=None explicitly to trigger those checks but end up with label='right'. I think it's best to rely on the default handling.

I'll remove the label is None handling in degradation.py and plotting.py. I'll also update the tests that specifically trigger this. Commit 0fe8ebd

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

return Rd_pct


def _avg_timestamp_old_Pandas(dt, dt_left):
Copy link
Collaborator

Choose a reason for hiding this comment

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

With pandas three now released, should we think about dropping support for pandas 1.x.x?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, maybe we start with a deprecation warning for pandas < 2.0 in our next release. Pandas 2.0 was only put out in 2023 so we can probably hang on to this functionality for a few more years - its not really causing any harm. PVLib is still on pandas > 1.3.3 so we should probably still support it too...

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.

4 participants