update spectral_factor_firstsolar #2100

RDaxini · 2024-06-21T11:46:05Z

Main change: update data screens in spectrum.spectral_factor_firstsolar

Closes spectral_factor_firstsolar changes user input data #2086 , relates to project Google Summer of Code project summary: Implementing new spectral corrections in pvlib #2065
I am familiar with the contributing guidelines
Tests added
Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/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`).
New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
Pull request is nearly complete and ready for detailed review.
Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

Summary of changes:

Added parameters for min/max AMa
Changed handling of values exceeding max pw
Replaced subjective phrasing of warnings
Added warning for low AMa
Revised docs, e.g. comments on model divergence moved to notes

Feedback on #2086 and recommendations for alternative/additional updates more than welcome.

Added parameters to enable user to set min/max airmass Changed handling of values exceeding max pw to default to max pw rather than np.nan Replaced subjective phrasing such as "exceptionally high" with "high" and removed references to "divergence" in the warnings in order to align with the fact that the user can now determine their own limits, which may or may not be "exceptionally" high/low or cause model divergence Added tests for new AMa limit parameters, modified warnings and expected values in existing tests

RDaxini · 2024-06-21T14:16:21Z

@kandersolar is the failed test the one that you mentioned before fails randomly sometimes?

AdamRJensen · 2024-06-21T14:18:44Z

@kandersolar is the failed test the one that you mentioned before fails randomly sometimes?

@RDaxini Yes it's unrelated to your contribution.

pvlib/spectrum/mismatch.py

cwhanse · 2024-06-21T15:47:46Z

removed references to "divergence" in the warnings

I would rather see this information moved to a Notes section, than deleted. The reason for the bounds should be retained. For PW, the bounds ensure that the model doesn't "diverge" from physically reasonable values, e.g., if pw=0.01 then the last term involving $\frac{1}{\sqrt{pw}}$ could cause the modifier to fall outside of [0, 1].

pvlib/spectrum/mismatch.py

RDaxini · 2024-07-03T12:18:07Z

 Based on the PVLIB Matlab function ``pvl_FSspeccorr`` (...)

Is this statement referring to the matlab function really necessary?

cwhanse · 2024-07-03T16:41:31Z

 Based on the PVLIB Matlab function ``pvl_FSspeccorr`` (...)
Is this statement referring to the matlab function really necessary?

Not any longer, this reference can be removed.

docs/sphinx/source/whatsnew/v0.11.1.rst

AdamRJensen

I'm surprised that a minimum airmass is added. Where does the default value come from (a quote or screen-shot from the paper perhaps)?

pvlib/spectrum/mismatch.py

adriesse + AdamRJensen review Co-authored-by: Anton Driesse <[email protected]> Co-authored-by: Adam R. Jensen <[email protected]>

pvlib/spectrum/mismatch.py

Co-authored-by: Anton Driesse <[email protected]>

pvlib/tests/test_spectrum.py

plus removed a few commas, capitalised a letter

RDaxini · 2024-07-10T17:30:55Z

All good from my side now save for this one comment --- is the whatsnew entry for this PR okay?

docs/sphinx/source/whatsnew/v0.11.1.rst

pvlib/spectrum/mismatch.py

Co-authored-by: Adam R. Jensen <[email protected]>

AdamRJensen

A few nitpicky things:

Could you write out precipitable water in the warnings (minor, feel free to ignore if you disagree)
Add a sentence in the Notes section, stating that the min/max values are from the original pvlib implementation to the author (consider adding a link to the PR). I think this is important info as most users aren't going to find this discussion.

pvlib/spectrum/mismatch.py

pvlib/tests/test_spectrum.py

Co-authored-by: Adam R. Jensen <[email protected]>

RDaxini · 2024-07-12T17:16:41Z

@AdamRJensen thanks for the comments. Those suggestions are fine, I have implemented them.
+I will fix tests shortly

pw -> precipitable water

pvlib/spectrum/mismatch.py

Co-authored-by: Echedey Luis <[email protected]>

AdamRJensen · 2024-07-17T14:51:08Z

Thank you @RDaxini for some great GSoC contributions!

RDaxini mentioned this pull request Jun 21, 2024

spectral_factor_firstsolar changes user input data #2086

Closed

cwhanse reviewed Jun 21, 2024

View reviewed changes

pvlib/spectrum/mismatch.py Show resolved Hide resolved

RDaxini added 2 commits June 24, 2024 11:37

out of limit input handling

be7a798

Update mismatch.py

d6ef34b

kandersolar reviewed Jun 26, 2024

View reviewed changes

pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved

kandersolar added this to the v0.11.1 milestone Jun 27, 2024

kandersolar added enhancement GSoC Contributions related to Google Summer of Code. labels Jun 27, 2024

RDaxini added 3 commits June 28, 2024 20:36

Update mismatch.py

6b5f1b6

Update test_spectrum.py

70c832a

Update mismatch.py

ed4a973

RDaxini added 4 commits July 3, 2024 20:43

Update mismatch.py

4157f7f

Create v0.11.1.rst

16d266f

Merge branch 'main' into update_firstsolar_scf

e7cc7d0

Update v0.11.1.rst

e373a70

RDaxini commented Jul 3, 2024

View reviewed changes

docs/sphinx/source/whatsnew/v0.11.1.rst Show resolved Hide resolved

RDaxini mentioned this pull request Jul 4, 2024

Google Summer of Code project summary: Implementing new spectral corrections in pvlib #2065

Closed

AdamRJensen reviewed Jul 5, 2024

View reviewed changes

pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved

pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved

adriesse requested changes Jul 5, 2024

View reviewed changes

pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved

pvlib/spectrum/mismatch.py Show resolved Hide resolved

Apply suggestions from code review

3cb6c75

adriesse + AdamRJensen review Co-authored-by: Anton Driesse <[email protected]> Co-authored-by: Adam R. Jensen <[email protected]>

adriesse reviewed Jul 5, 2024

View reviewed changes

pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved

RDaxini and others added 2 commits July 7, 2024 12:07

Update pvlib/spectrum/mismatch.py

ca6ddf6

Co-authored-by: Anton Driesse <[email protected]>

Merge remote-tracking branch 'upstream/main' into update_firstsolar_scf

9d171f0

kandersolar reviewed Jul 9, 2024

View reviewed changes

pvlib/tests/test_spectrum.py Show resolved Hide resolved

RDaxini added 3 commits July 10, 2024 13:14

create

755eb3d

Update test_spectrum.py

24ec55b

module_type typesetting

fc3e784

plus removed a few commas, capitalised a letter

cwhanse approved these changes Jul 10, 2024

View reviewed changes

docs/sphinx/source/whatsnew/v0.11.1.rst Show resolved Hide resolved

AdamRJensen reviewed Jul 11, 2024

View reviewed changes

pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved

RDaxini and others added 2 commits July 11, 2024 11:10

Update pvlib/spectrum/mismatch.py

f390429

Co-authored-by: Adam R. Jensen <[email protected]>

Merge remote-tracking branch 'upstream/main' into update_firstsolar_scf

b556456

AdamRJensen approved these changes Jul 12, 2024

View reviewed changes

pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved

pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved

pvlib/tests/test_spectrum.py Outdated Show resolved Hide resolved

RDaxini and others added 2 commits July 12, 2024 17:39

Apply suggestions from code review

51b31bc

Co-authored-by: Adam R. Jensen <[email protected]>

Update notes

341aafa

review comments

5ee902a

pw -> precipitable water

echedey-ls reviewed Jul 12, 2024

View reviewed changes

pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved

pvlib/spectrum/mismatch.py Outdated Show resolved Hide resolved

RDaxini and others added 2 commits July 15, 2024 17:21

Update pvlib/spectrum/mismatch.py

046c67d

Co-authored-by: Echedey Luis <[email protected]>

Update mismatch.py

80fe9e8

RDaxini closed this Jul 15, 2024

RDaxini reopened this Jul 15, 2024

RDaxini added 2 commits July 15, 2024 17:34

Update mismatch.py

397d1dc

Update mismatch.py

a180793

AdamRJensen merged commit 2e9c2c0 into pvlib:main Jul 17, 2024
30 checks passed

RDaxini deleted the update_firstsolar_scf branch July 17, 2024 16:51

echedey-ls mentioned this pull request Jul 18, 2024

restructure pvlib/spectrum #2136

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update spectral_factor_firstsolar #2100

update spectral_factor_firstsolar #2100

RDaxini commented Jun 21, 2024 •

edited

Loading

RDaxini commented Jun 21, 2024

AdamRJensen commented Jun 21, 2024

cwhanse commented Jun 21, 2024 •

edited

Loading

RDaxini commented Jul 3, 2024

cwhanse commented Jul 3, 2024

AdamRJensen left a comment

RDaxini commented Jul 10, 2024

AdamRJensen left a comment

RDaxini commented Jul 12, 2024

AdamRJensen commented Jul 17, 2024

update spectral_factor_firstsolar #2100

update spectral_factor_firstsolar #2100

Conversation

RDaxini commented Jun 21, 2024 • edited Loading

RDaxini commented Jun 21, 2024

AdamRJensen commented Jun 21, 2024

cwhanse commented Jun 21, 2024 • edited Loading

RDaxini commented Jul 3, 2024

cwhanse commented Jul 3, 2024

AdamRJensen left a comment

Choose a reason for hiding this comment

RDaxini commented Jul 10, 2024

AdamRJensen left a comment

Choose a reason for hiding this comment

RDaxini commented Jul 12, 2024

AdamRJensen commented Jul 17, 2024

RDaxini commented Jun 21, 2024 •

edited

Loading

cwhanse commented Jun 21, 2024 •

edited

Loading