[WIP] Attempt refactor soiling pr by martin-springer · Pull Request #479 · NatLabRockies/rdtools

martin-springer · 2026-02-05T01:00:43Z

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

…tio and fit multiple soiling rates per soiling interval (piecewise)) as well as CODS algorithm being added

…rials' into development

Signed-off-by: nmoyer <noah.moyer@nrel.gov>

Move SRR and CODS development branch from noromo01 to rdtools repo

…nto dev_SRR_CODS

… affected

…bare_except_error

…or_soiling_pr

codecov-commenter · 2026-02-12T21:18:39Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.09%. Comparing base (acc357c) to head (2fa047c).

Additional details and impacted files

@@               Coverage Diff               @@
##           development     #479      +/-   ##
===============================================
- Coverage        96.18%   96.09%   -0.09%     
===============================================
  Files               12       12              
  Lines             2280     2458     +178     
===============================================
+ Hits              2193     2362     +169     
- Misses              87       96       +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull request overview

This is a work-in-progress pull request that refactors and enhances the soiling module in rdtools, specifically the SRR (Stochastic Rate and Recovery) and CODS (Combined Degradation and Soiling) algorithms. The PR removes the experimental warning label and introduces several new features for detecting negative shifts and piecewise linear fitting in soiling analysis.

Changes:

Renamed parameters for clarity: min_interval_length → min_interval_days, max_negative_step → max_neg_step
Added negative shift detection (detect_neg_shifts) and piecewise linear fitting (piecewise_fit) capabilities
Added inferred_clean method for handling detected cleaning events with inferred recovery values
Fixed pandas Copy-on-Write compatibility issue and variable shadowing bug
Added comprehensive test coverage for new features with new fixtures for negative shifts and piecewise slopes
Removed experimental warnings from soiling module functions

Reviewed changes

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

Show a summary per file

File	Description
rdtools/soiling.py	Major refactor: parameter renames, new features (detect_neg_shifts, piecewise_fit, inferred_clean method), bug fixes (CoW, variable shadowing), added segmented_soiling_period function
rdtools/test/soiling_test.py	Updated tests for parameter renames, added tests for new features (negative shifts, piecewise fitting), fixed typos, reformatted to double quotes
rdtools/test/soiling_cods_test.py	Added new tests for CODS edge cases (NaN prefix handling, non-daily frequency, invalid order, prescient_cleaning_events mismatch)
rdtools/test/conftest.py	Added two new fixtures: soiling_normalized_daily_with_neg_shifts and soiling_normalized_daily_with_piecewise_slope
rdtools/plotting.py	Removed experimental warnings from soiling plotting functions, removed unused warnings import
docs/sphinx/source/changelog/pending.rst	Comprehensive changelog documenting all breaking changes, API changes, enhancements, bug fixes, and testing updates
docs/sphinx/source/changelog.rst	Added include for pending.rst
docs/sphinx/source/examples.rst	Added reference to new soiling_options_guide notebook
docs/sphinx/source/examples/soiling_options_guide.nblink	New notebook link file for soiling options guide
docs/degradation_and_soiling_example.ipynb	Updated outputs to show new columns (inferred_recovery, inferred_begin_shift) in soiling interval summary
docs/TrendAnalysis_example_NSRDB.ipynb	Minor formatting fix (removed semicolon)
.github/workflows/nbval.yaml	Added new notebook to test matrix, fixed pytest command syntax

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

rdtools/test/soiling_test.py

rdtools/soiling.py

rdtools/test/soiling_test.py

rdtools/soiling.py

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 12 out of 17 changed files in this pull request and generated 6 comments.

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

Copilot · 2026-02-12T23:53:07Z

.github/workflows/nbval.yaml

      run: |
        # --sanitize-with:  pre-process text to remove irrelevant differences (e.g. warning filepaths)
-        pytest --nbval docs/${{ matrix.notebook-file }} --sanitize-with docs/nbval_sanitization_rules.cfg
+        pytest --nbval --nbval-sanitize-with docs/nbval_sanitization_rules.cfg docs/${{ matrix.notebook-file }}


The workflow command uses --nbval-sanitize-with, but the repository docs (and the comment immediately above) reference nbval’s --sanitize-with option. With nbval>=0.10.0 in test dependencies, --nbval-sanitize-with is likely not a valid flag and would break the notebook CI job. Consider switching back to --sanitize-with docs/nbval_sanitization_rules.cfg (or updating the docs/comment and nbval requirement if this flag is intentional).

Suggested change

pytest --nbval --nbval-sanitize-with docs/nbval_sanitization_rules.cfg docs/${{ matrix.notebook-file }}

pytest --nbval --sanitize-with docs/nbval_sanitization_rules.cfg docs/${{ matrix.notebook-file }}

Copilot · 2026-02-12T23:53:07Z

rdtools/test/soiling_test.py

 def _build_monthly_summary(top_rows):
-    '''
+    """
    Convienience function to build a full monthly soiling summary


Typo in docstring: "Convienience" should be "Convenience".

Suggested change

Convienience function to build a full monthly soiling summary

Convenience function to build a full monthly soiling summary

Copilot · 2026-02-12T23:53:07Z

rdtools/soiling.py

+        if not detect_neg_shifts:
+            results.loc[filt, "valid"] = False


When detect_neg_shifts=True, intervals that fail the slope validity criteria (run_slope > 0 or slope_err too large) have their slopes zeroed, but valid is never set to False. This means downstream logic (e.g., selecting results[results.valid] and reporting soiling_ratio_perfect_clean) will treat these excluded intervals as valid. Consider setting results.loc[filt, "valid"] = False (and any other validity-related fields) regardless of detect_neg_shifts, while still skipping the max_neg_step criterion when detect_neg_shifts=True.

Suggested change

if not detect_neg_shifts:

results.loc[filt, "valid"] = False

results.loc[filt, "valid"] = False

Copilot · 2026-02-12T23:53:08Z

rdtools/soiling.py

+    # Initialize default for initial_guesses to avoid mutable default argument
+    if initial_guesses is None:
+        initial_guesses = [13, 1, 0, 0]
+
+    # Define bounds if not provided
+    if bounds is None:
+        # bounds are neg in first 4 and pos in second 4
+        # ordered as x0,b,k1,k2 where x0 is the breakpoint k1 and k2 are slopes
+        bounds = [(13, -5, -np.inf, -np.inf), ((len(pr) - 13), 5, +np.inf, +np.inf)]


segmented_soiling_period takes days_clean_vs_cp as a parameter, but the default bounds hard-code the change-point search window to start/end at 13 days (lower bound x0=13, upper bound len(pr)-13). If a caller passes a different days_clean_vs_cp, the bounds will not reflect that and can incorrectly prevent valid change points from being found. Consider deriving the default x0 bounds from days_clean_vs_cp (and validating that len(pr) is large enough).

Suggested change

# Initialize default for initial_guesses to avoid mutable default argument

if initial_guesses is None:

initial_guesses = [13, 1, 0, 0]

# Define bounds if not provided

if bounds is None:

# bounds are neg in first 4 and pos in second 4

# ordered as x0,b,k1,k2 where x0 is the breakpoint k1 and k2 are slopes

bounds = [(13, -5, -np.inf, -np.inf), ((len(pr) - 13), 5, +np.inf, +np.inf)]

n_points = len(pr)

# Initialize default for initial_guesses to avoid mutable default argument

if initial_guesses is None:

initial_guesses = [13, 1, 0, 0]

# Derive minimum distance from series ends using days_clean_vs_cp

min_distance = int(days_clean_vs_cp)

if min_distance < 1:

raise ValueError("days_clean_vs_cp must be at least 1")

# If the series is too short for the requested clean-vs-change-point window,

# return a NaN series and no change point (consistent with other failure modes).

if n_points <= 2 * min_distance:

z = [np.nan] * n_points

cp_index = None

sr = pd.Series(z, index=pr.index)

return sr, cp_index

# Define bounds if not provided

if bounds is None:

# bounds are neg in first 4 and pos in second 4

# ordered as x0,b,k1,k2 where x0 is the breakpoint k1 and k2 are slopes

bounds = [(min_distance, -5, -np.inf, -np.inf),

(n_points - min_distance, 5, +np.inf, +np.inf)]

Copilot · 2026-02-12T23:53:08Z

rdtools/soiling.py

-                                   if ce-index+HW not in index_dummy]
+            if np.abs(u[0]) > np.sqrt(f.R) / 2:
+                index_dummy = [n + 3 for n in range(window_size - HW - 1) if n + 3 != HW]
+                cleaning_events = [


Variable cleaning_events is not used.

Suggested change

cleaning_events = [

cleaning_events[:] = [

Copilot · 2026-02-12T23:53:08Z

rdtools/soiling.py

+                    if changepoint is False:
+                        prev_shift = start_shift  # assigned at new soil period
+
+                elif new_soil > 0:  # within soiling period


Test is always true, because of this condition.

Suggested change

elif new_soil > 0: # within soiling period

else: # within soiling period

nmoyer and others added 30 commits June 24, 2024 11:57

Matt’s updates to SRR algorithm (detect negative shifts in soiling ra…

c8aae89

…tio and fit multiple soiling rates per soiling interval (piecewise)) as well as CODS algorithm being added

committing updates to merge with aggregated_filters_for_trials

e1401a6

Making sure there will be no merge conflicts

f23a497

Merge remote-tracking branch 'remotes/origin/aggregated_filters_for_t…

30f5548

…rials' into development

Improvements in order to pass checks and pytesting

9e3a411

Signed-off-by: nmoyer <noah.moyer@nrel.gov>

Merge pull request #417 from noromo01/development

9e3d89d

Move SRR and CODS development branch from noromo01 to rdtools repo

Merge remote-tracking branch 'origin/aggregated_filters_for_trials' i…

c08a0e9

…nto dev_SRR_CODS

formatting conftest.py and soiling_test.py

35a3ec9

fixing formatting

3fdf0b0

lint soiling.py

669ec75

lint line length

23710a0

revert TrendAnalysis notebook changes

fa1d79b

revert conftest.py

d642210

revert notebook requirements

9122b56

added piecewise and neg_shift PI data back to conftest.py

cd4fbb6

formatting fixes

e9a2552

minor formatting issue in soiling.py

612c9f1

testing some changes to pass notebook checks

0f020b5

trying another minor change for notebook checks

6d5ce23

soiling.py change to pass notebook checks

b99c2de

Trying some changes in the notebooks to pass tests

ab28608

Fixing pytests and reverting notebooks

2dbbeae

undoing some black formatting

febe693

cleaning up formatting redundancies in soiling_test.py

ca7627b

reformatting soiling.py and minor reformatting to soiling_test.py

8b3fa4a

run black on soiling.py

efa5042

fixing flake8 formatting

21da67d

fixing flake8 formatting

5ef6c81

removing _collapse_cleaning_events so half_norm_clean results are not…

e66c295

… affected

fixing notebook failures

628cfe8

martin-springer and others added 8 commits December 12, 2024 16:49

Merge branch 'development' into qnguyen345-bare_except_error

9b1fd4c

Merge remote-tracking branch 'remotes/origin/master' into qnguyen345-…

a0a131f

…bare_except_error

formatting and syntax changes

af956a5

attempt refactor

3ce5572

update soiling profiles

54cf8fa

Merge remote-tracking branch 'origin/development' into attempt_refact…

99cad3f

…or_soiling_pr

Merge branch 'development' into attempt_refactor_soiling_pr

892820d

fix refactor changes in tests

a69d01b

martin-springer added 10 commits February 12, 2026 16:36

create a pending changelog

3cc2926

fix pandas 2 vs 3 error

b996d42

remove changelog entries from v3.0.0 and added them to pending

0b15c65

improve test coverage

54a4df1

fix segmented_soiling_period regression error

e3a3be6

draft notebook for soiling options

04aab1a

add the soiling options notebook to examples

87b9200

update the comparision notebook

303326a

linting

eccddb1

add soiling notebook to nbval

1fddd93

martin-springer requested a review from Copilot February 12, 2026 22:51

Copilot started reviewing on behalf of martin-springer February 12, 2026 23:03 View session

Copilot AI reviewed Feb 12, 2026

View reviewed changes

martin-springer and others added 5 commits February 12, 2026 18:17

Update rdtools/test/soiling_test.py

c6afc14

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update rdtools/soiling.py

d738909

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update rdtools/soiling.py

474fd6d

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update rdtools/test/soiling_test.py

b93d2c1

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

implement copilot suggestions

2fa047c

martin-springer requested a review from Copilot February 12, 2026 23:44

Copilot started reviewing on behalf of martin-springer February 12, 2026 23:45 View session

Copilot AI reviewed Feb 12, 2026

View reviewed changes

	pytest --nbval --nbval-sanitize-with docs/nbval_sanitization_rules.cfg docs/${{ matrix.notebook-file }}
	pytest --nbval --sanitize-with docs/nbval_sanitization_rules.cfg docs/${{ matrix.notebook-file }}

	Convienience function to build a full monthly soiling summary
	Convenience function to build a full monthly soiling summary

	if not detect_neg_shifts:
	results.loc[filt, "valid"] = False
	results.loc[filt, "valid"] = False

	elif new_soil > 0: # within soiling period
	else: # within soiling period

Conversation

martin-springer commented Feb 5, 2026

Uh oh!

codecov-commenter commented Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Feb 12, 2026 •

edited

Loading