Multi yoy no label#498
Conversation
|
OK, so it looks like taking the median of every slope whose center-point falls within the moving window will re-create the original |
|
.. But with the multi-YoY output, taking the median of all slopes rather than a pointwise average will result in much more smoothing in the center of the timeseries (where you have more slopes available), but more variability at start and end of the timeseries when you're only averaging the 1-yr slopes. |
Agreed, with the multi-yoy approach the time series plot becomes less meaningful as it kind of converges to the overall system degradation rate in the center of the plot and that's kind of unrelated to the years shown in the x-labels. How about, for plotting purposes, we only use the 1-year slopes (slopes <1.5 year to account for leap years) for the whole plot? Would this give us a better representation of the system degradation along the time axis? |
|
We could leverage the |
…ars, removing warning.
|
OK, I've added a filtering step where only slopes <= 2 years + 1 day (731 days) are included in the median. This helps to avoid over-smoothing of the multi-YoY from having too many slope values. I think this is a good compromise. I decided against making the filtering based on the width of the moving window because you could quickly filter out all valid slopes. |
|
would this give us the behavior we're after? cdeline#1 @cdeline @martin-springer let me know what you think. |
There was a problem hiding this comment.
Pull request overview
This PR updates the year-on-year (YoY) degradation workflow to remove reliance on label=-based timestamping, adds explicit YoY timestamp metadata (YoY_times) to the degradation_year_on_year outputs, and refactors the degradation time-series plotting path to better support multi-YoY results.
Changes:
- Removes
label=-specific tests/fixtures and updatesTrendAnalysisdefaults to no longer passlabelthroughyoy_kwargs. - Extends YoY calc output metadata with
calc_info['YoY_times']and updatesdegradation_timeseries_plotto use timestamps/durations from that structure. - Updates v3.2.0 changelog entries to reflect the new
YoY_timesoutput and the revised multi-YoY time-series plotting approach.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| rdtools/test/plotting_test.py | Removes label-based coverage and updates multi-YoY handling assertions for degradation_timeseries_plot. |
| rdtools/test/degradation_test.py | Removes tests for now-removed/changed label='center' behavior. |
| rdtools/plotting.py | Refactors degradation_timeseries_plot to consume YoY_times and handle multi-YoY smoothing differently. |
| rdtools/degradation.py | Removes label documentation/logic and adjusts how YoY timestamps/indices are produced. |
| rdtools/analysis_chains.py | Removes default yoy_kwargs={"label": "right"} so chains don’t depend on the label parameter. |
| docs/sphinx/source/changelog/v3.2.0.rst | Updates changelog notes for YoY_times and the new multi-YoY plotting behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Ensure the data is in order | ||
| energy_normalized = energy_normalized.sort_index() | ||
| energy_normalized.name = 'energy' | ||
| energy_normalized.index.name = 'dt' | ||
|
|
| YoY_times = YoY_times[['dt', 'dt_center', 'dt_left']] | ||
| YoY_times = YoY_times.rename(columns={'dt': 'dt_right'}) | ||
|
|
||
| YoY_times.set_index(YoY_times[f'dt_{label}'], inplace=True) | ||
| YoY_times.index.name = 'dt' | ||
|
|
||
| # now apply either right, left, or center label index to the yoy_result | ||
| yoy_result.index = YoY_times[f'dt_{label}'] | ||
| # now apply right label index to the yoy_result | ||
| yoy_result.index = YoY_times.index |
| yoy_info : dict | ||
| a dictionary with keys: | ||
| * YoY_values - pandas series of year on year slopes | ||
| * YoY_times - pandas series of corresponding timestamps |
| try: | ||
| results_values = yoy_info['YoY_values'] | ||
| results = yoy_info['YoY_times'].join(yoy_info['YoY_values'].rename('yoy')) | ||
| except KeyError: | ||
| raise KeyError("yoy_info input dictionary does not contain key `YoY_values`.") | ||
| raise KeyError("yoy_info input dictionary does not contain keys" | ||
| " `YoY_times` and `YoY_values`.") |
| # test defaults | ||
| result_right = degradation_timeseries_plot(yoy_info) | ||
| assert_isinstance(result_right, plt.Figure) | ||
| xlim_right = result_right.get_axes()[0].get_xlim()[0] | ||
|
|
||
| # test label='center' | ||
| result_center = degradation_timeseries_plot(yoy_info=degradation_info_center[3], | ||
| include_ci=False) | ||
| assert_isinstance(result_center, plt.Figure) | ||
| xlim_center = result_center.get_axes()[0].get_xlim()[0] | ||
|
|
||
| # test label='left' | ||
| result_left = degradation_timeseries_plot(yoy_info=degradation_info_left[3], | ||
| include_ci=False) | ||
| assert_isinstance(result_left, plt.Figure) | ||
| xlim_left = result_left.get_axes()[0].get_xlim()[0] | ||
|
|
||
| # test default label matches label='right' | ||
| result_default = degradation_timeseries_plot(yoy_info=yoy_info, include_ci=False) | ||
| xlim_default = result_default.get_axes()[0].get_xlim()[0] | ||
| assert xlim_default == xlim_right | ||
|
|
||
| # Check that the xlim values are offset as expected | ||
| # right > center > left (since offset_days increases) | ||
| assert xlim_right > xlim_center > xlim_left | ||
|
|
||
| # The expected difference from right to left is 365 days (1 yrs), allow 5% tolerance | ||
| expected_diff = 365 | ||
| actual_diff = (xlim_right - xlim_left) | ||
| tolerance = expected_diff * 0.05 | ||
| assert abs(actual_diff - expected_diff) <= tolerance, \ | ||
| f"difference of right-left xlim {actual_diff} not within 5% of 1 yr." | ||
|
|
||
| # The expected difference from right to center is 182 days, allow 5% tolerance | ||
| expected_diff2 = 182 | ||
| actual_diff2 = (xlim_right - xlim_center) | ||
| tolerance2 = expected_diff2 * 0.05 | ||
| assert abs(actual_diff2 - expected_diff2) <= tolerance2, \ | ||
| f"difference of right-center xlim {actual_diff2} not within 5% of 1/2 year." | ||
| result_right.get_axes()[0].get_xlim()[0] | ||
|
|
__init__.pylabel=