Soiling algorithm updates #435

martin-springer · 2024-11-06T14:43:38Z

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 <[email protected]>

Move SRR and CODS development branch from noromo01 to rdtools repo

…nto dev_SRR_CODS

… affected

…nctoin

…nto bare_except_error

codecov-commenter · 2024-11-06T15:11:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.03%. Comparing base (596f584) to head (9b1fd4c).
Report is 136 commits behind head on development.

Additional details and impacted files

@@               Coverage Diff               @@
##           development     #435      +/-   ##
===============================================
- Coverage        96.19%   96.03%   -0.17%     
===============================================
  Files               11       11              
  Lines             2209     2370     +161     
===============================================
+ Hits              2125     2276     +151     
- Misses              84       94      +10

☔ 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.

…rror' into qnguyen345-bare_except_error

martin-springer · 2024-11-08T22:28:47Z

Seems like the pytest.PytestRemovedIn8Warning: in the degradation_and_soiling_example notebook might be related to nbval < 0.10 computationalmodelling/nbval#180

However, nbval greater than 0.9.6 has a bug with semicolon computationalmodelling/nbval#194

@mdeceglie - I believe you removed most of the semicolons from the notebooks? If so, we could try using a newer version of nbval.

mdeceglie · 2024-11-08T22:50:39Z

There are still a few semicolons, I'm fine with attempting to remove them and updating nbval

martin-springer · 2024-11-08T20:45:23Z

rdtools/soiling.py

+                "max_neg_step": min(run.delta),
+                "start_loss": 1,
+                "inferred_start_loss": run.pi_norm.median(),  # changed from mean/Matt
+                "inferred_end_loss": run.pi_norm.median(),  # changed from mean/Matt


@mdeceglie - This change from .mean() to .median() seems to cause the RuntimeWarning:Mean of empty slice.
Seems counterintuitive but changing it back to mean get's rid of the warning...

We could add a check whether pi_norm is empty.
Also, I'm not sure why "inferred_start_loss" and "inferred_end_loss" are the same here?

I'm adding the following condition for now. If there's a better way , we can change it:

"inferred_start_loss": np.nan if run.pi_norm.isna().any() else run.pi_norm.median(), # changed from mean/Matt "inferred_end_loss": np.nan if run.pi_norm.isna().any() else run.pi_norm.median(), # changed from mean/Matt

Copilot

Pull Request Overview

This pull request updates the soiling algorithm by modifying dependencies, adding new testing fixtures, and refining plotting functions.

Updated nbval dependency and CLI flag for compatibility with the latest version
Introduced two new test fixtures for soiling normalization variations
Revised plotting functions to remove runtime warnings and standardize docstrings

Reviewed Changes

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

File	Description
setup.py	Removed the nbval version constraint, potentially affecting bug workarounds
rdtools/test/conftest.py	Added new fixtures for simulating soiling normalization with negative shifts and piecewise slopes
rdtools/plotting.py	Standardized docstrings and removed experimental warnings from plotting functions
.github/workflows/nbval.yaml	Updated the nbval CLI flag to match the new version's requirements

Files not reviewed (1)

docs/sphinx/source/changelog/v3.0.0-beta.0.rst: Language not supported

Comments suppressed due to low confidence (2)

setup.py:39

The removal of the nbval version constraint may reintroduce the semicolon bug noted in earlier versions. Confirm that the currently used nbval version resolves that issue.

    "nbval",

.github/workflows/nbval.yaml:32

Ensure that the new CLI flag '--nbval-sanitize-with' is supported by the nbval version in use and functions as expected compared to the previous '--sanitize-with' flag.

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

mdeceglie

Many minor comments, but my biggest point of discussion is how the the values for the new parameters neg_shift and picewise can be combined and how those combos can be combined with the new _complex cleaning options.

Must neg_shift and picewise always be turned on together? If not, how should the cleaning assumptions behave? If so, perhaps we can combine them into a single parameter.

Ideally I'd like to remove the _complex versions of the cleaning assumptions and instead have the existing versions check the values of neg_shift and piecewise and adjust their logic appropriately,

After addressing all this we should give the docs a once over to ensure everything appears the way we want.

mdeceglie · 2024-12-02T18:46:45Z

rdtools/soiling.py

-The soiling module is currently experimental. The API, results,
-and default behaviors may change in future releases (including MINOR
-and PATCH releases) as the code matures.
-'''
 from rdtools import degradation as RdToolsDeg


Let's change this to from rdtools import degradation and make the associated changes throughout

mdeceglie · 2024-12-02T18:48:19Z

rdtools/soiling.py

-            raise ValueError('Daily insolation series must have '
-                             'daily frequency')
+        if pd.infer_freq(self.insolation_daily.index) != "D":
+            raise ValueError("Daily insolation series must have " "daily frequency")


Suggested change

raise ValueError("Daily insolation series must have " "daily frequency")

raise ValueError("Daily insolation series must have daily frequency")

mdeceglie · 2024-12-02T18:48:40Z

rdtools/soiling.py

-                       outlier_factor=1.5):
-        '''
+            if pd.infer_freq(self.precipitation_daily.index) != "D":
+                raise ValueError("Precipitation series must have " "daily frequency")


Suggested change

raise ValueError("Precipitation series must have " "daily frequency")

raise ValueError("Precipitation series must have daily frequency")

mdeceglie · 2024-12-02T18:49:07Z

rdtools/soiling.py

-            raise ValueError('Daily performance metric series must have '
-                             'daily frequency')
+        if pd.infer_freq(self.pm.index) != "D":
+            raise ValueError("Daily performance metric series must have " "daily frequency")


Suggested change

raise ValueError("Daily performance metric series must have " "daily frequency")

raise ValueError("Daily performance metric series must have daily frequency")

mdeceglie · 2024-12-02T18:50:40Z

rdtools/soiling.py

+###############################################################################
+# all code below for new piecewise fitting in soiling intervals within srr/Matt
+###############################################################################


I think this comment can be deleted

mdeceglie · 2025-04-01T01:55:22Z

rdtools/test/soiling_test.py

+     ("perfect_clean_complex", True, True, 0.977116),
+     ("inferred_clean_complex", True, True, 0.975805)])


Can perfect_clean_complex and inferred_clean_complex be used with neg_shift OR piecewise? Or do both neg_shift and picewise have to be True together? If not, we should test update the doc strings in soiling.py to say "or", and augment the the test matrix to mix and match.

mdeceglie · 2025-04-01T01:58:49Z

rdtools/test/soiling_test.py

+     ("perfect_clean_complex", True, True, 0.966912),
+     ("inferred_clean_complex", True, True, 0.965565)])


Same questions as above regarding mixing and matching piecewise and neg_shift

mdeceglie · 2025-04-09T15:23:39Z

rdtools/soiling.py

-        '''
+    #######################################################################
+    # add neg_shift and piecewise to the following def/Matt
+    def run(self, reps=1000, day_scale=13, clean_threshold="infer", trim=False,


Let's consider what defaults we want (also for soiling_srr(), which should be aligned with the run method). Should we activate some of the new functionality by default?

mdeceglie · 2025-05-30T21:06:10Z

rdtools/soiling.py

+    method : str, {'half_norm_clean', 'random_clean', 'perfect_clean',
+         perfect_clean_complex,inferred_clean_complex} \
        default 'half_norm_clean'
+
        How to treat the recovery of each cleaning event

-        * 'random_clean' - a random recovery between 0-100%
+        * 'random_clean' - a random recovery between 0-100%,
+           pair with piecewise=False and neg_shift=False
        * 'perfect_clean' - each cleaning event returns the performance
-          metric to 1
+          metric to 1,
+          pair with piecewise=False and neg_shift=False
        * 'half_norm_clean' - The starting point of each interval is taken
          randomly from a half normal distribution with its mode (mu) at 1 and
          its sigma equal to 1/3 * (1-b) where b is the intercept of the fit to
-          the interval.
+          the interval,
+          pair with piecewise=False and neg_shift=False
+        *'perfect_clean_complex' - each detected clean event returns the
+          performance metric to 1 while negative shifts in the data or
+          piecewise linear fits result in no cleaning,
+          pair with piecewise=True and neg_shift=True
+        *'inferred_clean_complex' - at each detected clean event the
+          performance metric increases based on fits to the data while
+          negative shifts in the data or piecewise linear fits result in no
+          cleaning,
+          pair with piecewise=True and neg_shift=True


The updated doc string is not rendering correctly on read the docs

mdeceglie · 2025-05-30T21:08:23Z

rdtools/soiling.py

+        method : str, {'half_norm_clean', 'random_clean', 'perfect_clean',
+             perfect_clean_complex,inferred_clean_complex} \
+            default 'perfect_clean_complex'
+
            How to treat the recovery of each cleaning event

-            * 'random_clean' - a random recovery between 0-100%
+            * 'random_clean' - a random recovery between 0-100%,
+               pair with piecewise=False and neg_shift=False
            * 'perfect_clean' - each cleaning event returns the performance
-              metric to 1
+              metric to 1,
+              pair with piecewise=False and neg_shift=False
            * 'half_norm_clean' - The starting point of each interval is taken
              randomly from a half normal distribution with its mode (mu) at 1 and
              its sigma equal to 1/3 * (1-b) where b is the intercept of the fit to
-              the interval.
+              the interval,
+              pair with piecewise=False and neg_shift=False
+            * 'perfect_clean_complex' - each detected clean event returns the
+              performance metric to 1 while negative shifts in the data or
+              piecewise linear fits result in no cleaning,
+              pair with piecewise=True and neg_shift=True
+            * 'inferred_clean_complex' - at each detected clean event the
+              performance metric increases based on fits to the data while
+              negative shifts in the data or piecewise linear fits result in no
+              cleaning,
+              pair with piecewise=True and neg_shift=True


Updated doc string not appearing correctly in read the docs

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 <[email protected]>

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

Nguyen, Quyen and others added 6 commits October 24, 2024 13:37

added fix for bare except and pytests for segmented_soiling_period fu…

7517ddc

…nctoin

updated changelogs

cee6105

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

9734118

…nto bare_except_error

deleted updates in wrong changelogs

cfd5858

fixed lint error

6172ea0

remove soiling experimental warning label

693c570

martin-springer mentioned this pull request Nov 6, 2024

Soiling algorithm updates Quyen's branch #432

Closed

6 tasks

martin-springer added 3 commits November 6, 2024 09:46

Merge branch 'development' into qnguyen345-bare_except_error

b96ebcc

update changelog

e5159d7

rerun notebooks

fcac604

martin-springer added 8 commits November 8, 2024 12:06

Merge remote-tracking branch 'github-desktop-qnguyen345/bare_except_e…

6346480

…rror' into qnguyen345-bare_except_error

fix double quotations left over from merge

fd4dc47

remove double import

dc46cf6

remove experimental soiling warnings from plotting

8492873

remove warnings import

7df691c

quick fix mean runtimewarning

b0c4510

re-run notebooks

48f02dc

fix linting in soiling

0299804

martin-springer added 3 commits November 11, 2024 11:01

update nbval and remove semicolons from nb's

08e0090

re-run notebooks

ec105a3

nbval update sanitize-with argument

5ded716

martin-springer commented Nov 13, 2024

View reviewed changes

Merge branch 'development' into qnguyen345-bare_except_error

9b1fd4c

martin-springer mentioned this pull request Dec 18, 2024

Numpy deprecation warnings in soiling algorithm #445

Open

mdeceglie requested a review from Copilot April 23, 2025 18:25

Copilot AI reviewed Apr 23, 2025

View reviewed changes

mdeceglie requested changes May 30, 2025

View reviewed changes

	raise ValueError("Daily insolation series must have " "daily frequency")
	raise ValueError("Daily insolation series must have daily frequency")

	raise ValueError("Precipitation series must have " "daily frequency")
	raise ValueError("Precipitation series must have daily frequency")

	raise ValueError("Daily performance metric series must have " "daily frequency")
	raise ValueError("Daily performance metric series must have daily frequency")

		("perfect_clean_complex", True, True, 0.977116),
		("inferred_clean_complex", True, True, 0.975805)])

Soiling algorithm updates #435

Are you sure you want to change the base?

Soiling algorithm updates #435

Conversation

martin-springer commented Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

martin-springer commented Nov 8, 2024

Uh oh!

mdeceglie commented Nov 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

mdeceglie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

martin-springer commented Nov 6, 2024 •

edited

Loading

codecov-commenter commented Nov 6, 2024 •

edited

Loading