Add N. Martin & J. M. Ruiz spectral response mismatch modifiers model #1658

echedey-ls · 2023-02-08T23:04:14Z

I am familiar with the contributing guidelines
Tests added
Updates entries in docs/sphinx/source/reference for API changes.
New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
Pull request is nearly complete and ready for detailed review.
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`).
Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

Hi! As promised, I did a bigger PR.
This proposal adds a new function, pvlib.spectrum.martin_ruiz_spectral_modifier, with the spectral mismatch model proposed in this paper.
Details about the function can be read in the docstring, but to sum it up, it takes the type of material (m-Si, p-Si or a-Si), the absolute airmass and the clearness index to compute a small mismatch for each component's POA irradiance.
This PR has been done with the help of researchers at Instituto de Energía Solar, at Universidad Politécnica de Madrid.

There are a bit of things I'm unsure about, so I'd like to hear your opinion on them:

The name of the function
The tests names
Missing tests, if any comes to mind
Extensiveness of tests
Output keys
Behaviour when using parameter model_parameters (this is almost only to be used if anyone computes its model params)
Behaviour, in general. Hope submitted notebook spectral_mismatch_modifiers.ipynb and tests clarify the use.

Feel free to make comments on everything. English is not my mother tongue, so an in depth inspection of code comments and docs is also welcome.
Idk if I should add something else here, ask whatever you need, I will address it.

Doc links

Function docstring
Gallery example

cwhanse

I'm a bit puzzled by the omission of the leading term S_{ef Gbar(lambda)). As coded, this function returns the ratio S_{ef} / S_{ef Gbar(lambda)). Is this the appropriate factor to apply to the broadband irradiance?

pvlib/spectrum/mismatch.py

echedey-ls · 2023-02-09T18:41:22Z

I'm a bit puzzled by the omission of the leading term S_{ef Gbar(lambda)). As coded, this function returns the ratio S_{ef} / S_{ef Gbar(lambda)). Is this the appropriate factor to apply to the broadband irradiance?

Yes, you are right, the func is returning a ratio. In fact, it returns 3 of them, each one for a different POA component; this mismatch is for all the spectrum (with respect to the standard spectrum) response - it is independent from the wavelength.

kandersolar

Thanks for this excellent PR @echedey-ls! Well done all around -- code, docs, tests, etc. I plan to take a closer look later, but here are some comments until then.

Waiting for milestone assignment

I've assigned this to 0.9.5, so that's the relevant what's new file. You might need to update your branch with pvlib:main for it to exist.

Hope submitted notebook spectral_mismatch_modifiers.ipynb and tests clarify the use.

I think it is better to make a new gallery example here instead of a new notebook. We have talked about getting rid of the notebooks entirely. Also, a comment on the notebook code: I think the use of pvlib.irradiance.poa_components in cell 21 is not quite correct, since it is passing a poa_direct to dni, so the AOI projection will be incorrectly applied again.

pvlib/spectrum/mismatch.py

kandersolar · 2023-02-13T20:30:36Z

I'm a bit puzzled by the omission of the leading term S_{ef Gbar(lambda)). As coded, this function returns the ratio S_{ef} / S_{ef Gbar(lambda)). Is this the appropriate factor to apply to the broadband irradiance?

I am also a bit puzzled. I think, if this paper describes a factor suitable for multiplying with broadband irradiance, it must be this ratio. But its formulation, especially the integration limit in the denominator of Eq 1, seems strange to me. Need to chew a bit more on what this equation really represents.

echedey-ls · 2023-02-15T20:14:18Z

Thank you very much @kanderso-nrel ! I didn't know many of the things you spotted.

First, there is a duplicate of the Bug Fixes section in the original whatsnew for 0.9.5 on line 31, just in case you didn't notice; I did also not want to touch it.
Second, I don't know how to approach the plot_*.py example since the .ipynb tutorials are being deprecated. In the other .py files, I see many times that they replicate a plot from a paper. Should I make a script to mimic some plot in this Martin & Ruiz paper or can I just translate the notebook to this format? Also, if I want to add more code later to this example (e.g., for obtaining the custom model_parameters dict), could I just reuse the file or do I create a different one?
Regarding the questions about the paper, I will talk to my mentors and let you know in following days.

kandersolar · 2023-02-15T20:26:40Z

First, there is a duplicate of the Bug Fixes section in the original whatsnew for 0.9.5

Yes, there was a mistake in some other PR that created this duplication. We will fix that later so don't worry about it in this PR. For this PR it's fine to just add a new entry to Enhancements.

Should I make a script to mimic some plot in this Martin & Ruiz paper or can I just translate the notebook to this format? Also, if I want to add more code later to this example (e.g., for obtaining the custom model_parameters dict), could I just reuse the file or do I create a different one?

You can mimic a plot from the reference if you like. It would also be fine to just translate the notebook to this format. Basically the examples just need to show how to do something useful; it's up to you to decide what "useful" means ;) We do try to keep them relatively short, so if you want to show how to obtain custom model parameters, it may be better to create a new example file.

adriesse · 2023-02-18T20:32:44Z

pvlib/spectrum/mismatch.py

+    airmass_absolute : numeric
+        Absolute airmass. Give attention to algorithm used (``kasten1966`` is
+        recommended for default parameters of ``monosi``, ``polysi`` and
+        ``asi``, see [1]_).


If the model requires a specific air mass calculation, then perhaps it is better to have zenith angle as input.

This comment applies to a number of other existing pvlib functions as well, actually.

* Upgrade docstring

Following pvlib#1574

Now, even if not given parameters for a irradiation, a result is yield with value None.

commit 8f3b965 Author: echedey-ls <80125792+echedey-ls@users.noreply.github.com> Date: Fri Feb 3 19:25:42 2023 +0100 Update mismatch_modifiers.ipynb commit 3486069 Author: echedey-ls <80125792+echedey-ls@users.noreply.github.com> Date: Thu Feb 2 23:28:31 2023 +0100 Create mismatch_modifiers.ipynb commit 0747a43 Author: echedey-ls <80125792+echedey-ls@users.noreply.github.com> Date: Fri Jan 27 13:05:49 2023 +0100 I did fck it with the tmy_data.shift, never again commit 0e600b2 Author: echedey-ls <80125792+echedey-ls@users.noreply.github.com> Date: Tue Dec 6 17:12:04 2022 +0100 Example draft

Result new keys are ('poa_direct', 'poa_sky_diffuse', 'poa_ground_diffuse') For irradiances obtained thru pvlib.irradiance.poa_components

* doi backticks * cell_type --> module_type * spectrum.martin_ruiz_spectral_modifer --> spectrum.martin_ruiz * change hastattr __iter__ to np.isscalar * behaviour: providing module_type and model_parameters raises an error instead of a warning

* update tutorial

echedey-ls · 2023-02-19T23:30:41Z

I've added the sphinx example. I think this is the minimum needed to merge.
About the doubts that have arisen about the paper, I will try to address them this week with help of the mentors I've mentioned.
If I forgot something, don't hesitate to ask about it again.

@kanderso-nrel thank you very much for your guidance.

If the model requires a specific air mass calculation, then perhaps it is better to have zenith angle as input.

Thanks for the input @adriesse .
I wanted to keep the model as accurate as in the paper, to make it easy to understand from the document as possible. In my opinion, that would obfuscate it little more.
Also, this model introduces something like a 1% change in the incident sunlight, and the difference between 'kasten1966' and the default model 'kastenyoung1989' is almost negligible (see this Sandia page). I like to think that anyone using it will have seen the docs or the example at least, or even if they forgot the model, it won't almost have an impact in the results.

docs/examples/spectrum/plot_martin_ruiz_mismatch.py

in coordination with pvlib#1658, drop "spectral_correction" from the new function names

Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>

Co-Authored-By: Cliff Hansen <5393711+cwhanse@users.noreply.github.com>

pvlib/spectrum/mismatch.py

adriesse · 2023-02-24T19:59:52Z

pvlib/tests/test_spectrum.py

+    # Data to run tests of spectrum.martin_ruiz
+    kwargs = {
+        'clearness_index': [0.56, 0.612, 0.664, 0.716, 0.768, 0.82],
+        'airmass_absolute': [2, 1.8, 1.6, 1.4, 1.2, 1],


Have you checked the full range of possible kt and am values to see if the model behaves ok?

pvlib/spectrum/mismatch.py

adriesse · 2023-02-24T20:09:58Z

pvlib/spectrum/mismatch.py

+            [1.024, -.222, 920e-5, .840, -.728, -.0183, .989, -.219, .0179],
+        ])
+
+    # Argument validation and choose components and model parameters


Most functions in pvlib have less friendly input checking, which may be good or bad. You might just see what happens in each case without these checks and see whether the message python produces upon failure is comprehensible. In that case you may not need some of these custom messages.

Other than that I would prefer two nested if/else structures over the four combined conditions.

I have tried what you say, but I don't get nice results - when both are provided, one of the inputs is ignored without telling so; and when neither of them are, _params is not defined is raised, which could be a good error message.
In my humble opinion, being verbatim in the interface is not a problem, and is better than seeing a variable you did not write is not being defined. It will save time for the user, essentially.

numpy and scipy use the None, None default pattern, where if both are None the function fails, so I think we're among good company.

docs/examples/spectrum/plot_martin_ruiz_mismatch.py

0. We've found that MR model does not match other spectral mismatch models as expected - this commit shows that, in case some insight can be given 1. Uses atmosphere.first_solar_spectral_correction--> to be changed before merging 2. Compares with: * pvsystem.sapm_spectral_loss * atmosphere.first_solar_spectral_correction 3. Docs are already up-to-date with the first_solar renaming

Addressed in this commit: * docstring: use "must" * TypeError --> ValueError (func & tests updated) * Make use of a local TMY data file * Show per component maths behind modifier application Co-Authored-By: Anton Driesse <9001027+adriesse@users.noreply.github.com>

echedey-ls · 2023-02-25T00:20:28Z

I had a meeting with the profs I mentioned some time ago.
They were doubtful about the reliability / trustworthiness of this model. That's why I added the comparison section in the example. See built readthedocs example.
With that info, I'd like you to see it and determine if the Martin & Ruiz proposed method deserves being among PVLIB.

Have you checked the full range of possible kt and am values to see if the model behaves ok?

I started learning about PV almost by myself last summer as these profs asked me to, in order to use my knowledge in Python to try to make this addition possible. So please, bear with me for some of the scientific inconsistencies I might have done.
Answering @adriesse 's question, no, I didn't. That dataset is generated from a spreadsheet, so most tests only check mathematical workflow & I/O works as expected from the M&R paper. In fact, I do not have a clear path of how to approach the tests, if they are not suitable as of now (I mean, origin datasets, where to find them, how much error I should expect come to my mind right now). If the question only wants to address the range of input values, I can solve it, but it won't determine if it yields the values we want as a spectrum mismatch modifier.

The researchers at Instituto de Energía Solar of UPM, the profs, are trying to explain the difference compared to the other models and come to conclusions in following days/weeks. You will hear from me if I'm told anything.

Whether this PR should close, change to draft status or can be merged, is a decision I will leave up to you.

Edit: for the example I use func pvlib.atmosphere.first_solar_spectral_correction, before merging it will need to be renamed as stated by PR #1628. I've added a task in the PR description.

adriesse · 2023-02-25T09:44:18Z

The comparison in the example does look a bit odd. Anyway, don't give up. This would be an interesting addition to pvlib if the wrinkles can be sorted out. If problems remain with the model, then identifying those is scientific progress too!

echedey-ls · 2023-05-09T19:59:26Z

I had a little conversation with the researchers and I was told the model won't work for this purpose, so I'm closing this issue.

* migrate spectral modifiers to pvlib.spectrum * stickler * get fslr correction to 100% coverage * stickler! * change new names to sapm and first_solar in coordination with #1658, drop "spectral_correction" from the new function names * improve whatsnew * more renaming * rename to spectral_factor_X * tweaks * v0.9.5 -> v0.9.6 * 0.9.5 whatsnew cleanup * 0.9.6 -> 0.10.0 in deprecation warnings Co-Authored-By: Cliff Hansen <5393711+cwhanse@users.noreply.github.com> --------- Co-authored-by: Cliff Hansen <5393711+cwhanse@users.noreply.github.com>

pvlib/pvlib-python#1658

echedey-ls marked this pull request as ready for review February 9, 2023 13:37

cwhanse reviewed Feb 9, 2023

View reviewed changes

AdamRJensen added the enhancement label Feb 13, 2023

kandersolar added this to the 0.9.5 milestone Feb 13, 2023

kandersolar reviewed Feb 13, 2023

View reviewed changes

echedey-ls force-pushed the martin_ruiz_spectral_response_characterization branch from 79b8a7e to bebee82 Compare February 15, 2023 22:00

adriesse reviewed Feb 18, 2023

View reviewed changes

echedey-ls added 19 commits February 20, 2023 00:03

First prototype

a7d0070

Allow for custom irradiations in 'model_parameters'

60a63fd

Atomize tests with a fixture

93f1038

* Upgrade docstring

Comply with 'optional' instead of 'default None'

f606cd8

Following pvlib#1574

Update docstring

f79628a

Update docstring (2)

a7b47b4

Update comments

b9ec506

Change return & model_parameters behaviour

3a8630e

Now, even if not given parameters for a irradiation, a result is yield with value None.

Minor typos in docstrings

025c76e

Typo

8d4442e

Allow easier multiplication of modifiers and irradiances

399312b

Result new keys are ('poa_direct', 'poa_sky_diffuse', 'poa_ground_diffuse') For irradiances obtained thru pvlib.irradiance.poa_components

Show few days on example

f24aa92

Cant live without line length limit

c26b865

Rename notebook and specify spectral mismatch modifiers

900ed98

Delete plot_martin_ruiz_spectral_modifier_figs4to6.py

eb1b245

Little changes to docs and var names

149c557

Clean up & minor upgrades to notebook

2bf9d5b

Add output of notebook

be9554f

echedey-ls added 7 commits February 20, 2023 00:03

Exceptions: Delete exclamation marks as per code review

3d5cceb

docstring: update per code review (II)

b16eaa6

code review

f5352ac

* doi backticks * cell_type --> module_type * spectrum.martin_ruiz_spectral_modifer --> spectrum.martin_ruiz * change hastattr __iter__ to np.isscalar * behaviour: providing module_type and model_parameters raises an error instead of a warning

code review

d66f16e

* update tutorial

Add to reference and add whatsnew entry

db48a76

docstring: forgot to update raises

e7011ef

Add sphinx example and delete notebook

30c66b9

echedey-ls force-pushed the martin_ruiz_spectral_response_characterization branch from fbfc527 to 30c66b9 Compare February 19, 2023 23:04

cwhanse approved these changes Feb 20, 2023

View reviewed changes

kandersolar added a commit to kandersolar/pvlib-python that referenced this pull request Feb 20, 2023

change new names to sapm and first_solar

f90e0b9

in coordination with pvlib#1658, drop "spectral_correction" from the new function names

echedey-ls and others added 5 commits February 20, 2023 23:26

example: code review

c04b70d

Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>

example: code review (II)

183320f

Co-Authored-By: Cliff Hansen <5393711+cwhanse@users.noreply.github.com>

docstring: update per PR pvlib#1628

8730fcd

sapm_effective_irradiance: docstring rst typo

7ced74b

docstring: add math directives to parameters

6385897

adriesse requested changes Feb 24, 2023

View reviewed changes

echedey-ls and others added 2 commits February 24, 2023 23:30

kandersolar mentioned this pull request Mar 2, 2023

v0.9.5 release planning #1681

Closed

12 tasks

kandersolar modified the milestones: 0.9.5, 0.9.6 Mar 18, 2023

echedey-ls closed this May 9, 2023

kandersolar modified the milestones: 0.9.6, 0.10.0 May 16, 2023

echedey-ls added a commit to echedey-ls/MR-Spectrum that referenced this pull request Nov 27, 2023

Import plot & compare script from my PR of MR model at PVLIB's

9973f50

pvlib/pvlib-python#1658

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add N. Martin & J. M. Ruiz spectral response mismatch modifiers model #1658

Add N. Martin & J. M. Ruiz spectral response mismatch modifiers model #1658

echedey-ls commented Feb 8, 2023 •

edited

Loading

cwhanse left a comment

echedey-ls commented Feb 9, 2023

kandersolar left a comment

kandersolar commented Feb 13, 2023

echedey-ls commented Feb 15, 2023

kandersolar commented Feb 15, 2023

adriesse Feb 18, 2023 •

edited

Loading

echedey-ls commented Feb 19, 2023

adriesse Feb 24, 2023

adriesse Feb 24, 2023

echedey-ls Feb 24, 2023

cwhanse Feb 24, 2023

echedey-ls commented Feb 25, 2023 •

edited

Loading

adriesse commented Feb 25, 2023

echedey-ls commented May 9, 2023

Add N. Martin & J. M. Ruiz spectral response mismatch modifiers model #1658

Add N. Martin & J. M. Ruiz spectral response mismatch modifiers model #1658

Conversation

echedey-ls commented Feb 8, 2023 • edited Loading

Doc links

cwhanse left a comment

Choose a reason for hiding this comment

echedey-ls commented Feb 9, 2023

kandersolar left a comment

Choose a reason for hiding this comment

kandersolar commented Feb 13, 2023

echedey-ls commented Feb 15, 2023

kandersolar commented Feb 15, 2023

adriesse Feb 18, 2023 • edited Loading

Choose a reason for hiding this comment

echedey-ls commented Feb 19, 2023

adriesse Feb 24, 2023

Choose a reason for hiding this comment

adriesse Feb 24, 2023

Choose a reason for hiding this comment

echedey-ls Feb 24, 2023

Choose a reason for hiding this comment

cwhanse Feb 24, 2023

Choose a reason for hiding this comment

echedey-ls commented Feb 25, 2023 • edited Loading

adriesse commented Feb 25, 2023

echedey-ls commented May 9, 2023

echedey-ls commented Feb 8, 2023 •

edited

Loading

adriesse Feb 18, 2023 •

edited

Loading

echedey-ls commented Feb 25, 2023 •

edited

Loading