Add support for return_components to Reindl#2775
Conversation
Hey @cbcrespo! 🎉Thanks for opening your first pull request! We appreciate your If AI is used for any portion of this PR, you must vet the content |
kandersolar
added
enhancement
GSoC
…commas (pvlib#2771) read_nsrdb_psm4 split the three header lines with a naive str.split(','), which broke spectral-on-demand files whose column names are quoted fields containing commas (e.g. '"GaAs (Bauhuis et al., 2009)"'). Such names were split into spurious columns, raising on read. Parse the header lines with the csv module so quoted fields are kept intact. Fixes pvlib#2736
…lib#2777) * Update irradiance.py * Update test_irradiance.py * update sphinx refs, examples * Update v0.15.2.rst * flake8 * test deprecation * Update v0.15.2.rst
…pvlib#2752) * BUG: fix dirint KeyError with scalar inputs on pandas >= 2.0 * TST,CHANGELOG: add test and whatsnew entry for dirint scalar fix GH#2751 * BUG: fix dirint KeyError for scalar inputs on pandas >= 2.0 * STY: fix flake8 whitespace and blank line errors * BUG: add test coverage for dirint with array-like inputs GH#2751 * Update pvlib/irradiance.py Co-authored-by: Cliff Hansen <cwhanse@sandia.gov> * Update pvlib/irradiance.py Co-authored-by: Cliff Hansen <cwhanse@sandia.gov> * TST: fix test_dirint_array_inputs to use 2 time points * BUG: raise ValueError in dirint when use_delta_kt_prime=True and len(times)<2 * Update pvlib/irradiance.py Co-authored-by: Cliff Hansen <cwhanse@sandia.gov> * Update pvlib/irradiance.py Co-authored-by: Cliff Hansen <cwhanse@sandia.gov> * Update tests/test_irradiance.py Co-authored-by: Cliff Hansen <cwhanse@sandia.gov> * TST: remove orphaned times_single reference from dirint test * STY: fix line length and blank lines in dirint docstring --------- Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
* Update nasa_power VARIABLE_MAP with additional parameters - Added: ALLSKY_SFC_LW_DWN, ALLSKY_TOA_SW_DWN, CLRSKY_DIFF, PS, T2MDEW, T2MWET, RH2M, SRF_ALB, TQV - Fix PS unit conversion: kPa to Pa for pvlib.atmosphere compatibility - Fix parameter names: CLRSKY_DIFF -> CLRSKY_SFC_SW_DIFF, ALLSKY_TOA_SW_DWN -> TOA_SW_DWN, SRF_ALB -> ALLSKY_SRF_ALB - Drop T2MWET (duplicate target name breaks pandas rename) - Add TQV unit conversion: kg/m^2 to cm - Add regression tests for all VARIABLE_MAP params and unit conversions * Fix flake8 E501/E702 in test file * Address review comments: move unit conversion note to Notes section, simplify inline comments - Move unit conversion documentation from map_variables param description to a dedicated Notes section in the docstring - Simplify pressure conversion comment (remove specific function list) - Simplify TQV conversion comment (remove specific function reference) * Add dni_clear to variable map * Update notes section * Fix linter * Split test_get_nasa_power_all_variable_map_parameters_valid into two batches VARIABLE_MAP now has 16 entries, exceeding NASA POWER's 15-parameter-per-request limit. Split the test into two batches of 8 instead of asserting the limit. * Add whatsnew entry for nasa_power VARIABLE_MAP additions (PR pvlib#2762) * Address review: simplify whatsnew entry, add self to Contributors (PR pvlib#2762) * Apply suggestion from @echedey-ls Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com> * Apply suggestion from @AdamRJensen - combine whatsnew entries --------- Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com> Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
kandersolar
left a comment
kandersolar
left a comment
There was a problem hiding this comment.
Nice PR @cbcrespo! I think your interpretation of the horizon component is reasonable, although since it is not stated in the reference, I think we need to document it ourselves. A sentence in the Notes section of the docstring would be appropriate, I think.
I haven't yet added a test. I saw that e.g. haydavies has two separate tests, for return_components=False and return_components=True. However, from what I gathered, the test values came from the matlab implementation of pvlib, and therefore it seems kind of silly to create a test using values derived from the python implementation to test the python implementation. Curious as to what are the community's thoughts on this.
Yes, ideally the test values would come from an independent implementation. If you wanted, you could reimplement the model in Excel (or whatever you like) and calculate test values that way. In this case, I personally would just use the python implementation to compute the test values, for two reasons:
- the math is not very complicated, so we can be fairly confident that there is no bug in the code
- the test's intent is only partially to check that the correct numeric values are returned; equally important is that the test verifies that
return_components=Trueproduces a dict with the correct structure.
Also, a few comments below regarding whether the returned object should be OrderedDict or dict.
| sky_diffuse = dhi * (term1 + term2 + term3) | ||
|
|
||
| if return_components: | ||
| diffuse_components = OrderedDict() |
There was a problem hiding this comment.
I know this would be consistent with existing functions, but at some point we will switch those to normal dictionaries (see #1684). I suggest getting ahead of that change here and using a normal dictionary.
| The sky diffuse component of the solar radiation on a tilted | ||
| surface. | ||
|
|
||
| diffuse_components : OrderedDict (array input) or DataFrame (Series input) |
There was a problem hiding this comment.
If switching the return from OrderedDict to dict, change here as well
| ------- | ||
| poa_sky_diffuse : numeric | ||
| The sky diffuse component of the solar radiation. [Wm⁻²] | ||
| numeric, OrderedDict, or DataFrame |
@kandersolar just for clarification, this interpretation can be seen as factual rather than reasonable since, while it is not explicit in the paper, it is remaining term after subtracting the hay-davies models which predeces it (and to which reindl only adds an horizon term). @cbcrespo explanation of the interpretation is a bit convoluted because in the beginning we we're trying too much to make a physical interpretation of the term and we wanted to open up a bite the discussion. but then, when talking to Adam, we noticed that indeed this is simply the extra term when we multiply the multiplicative terms. Anyway, something can be added to the Notes if that is deemed relevant, but it should be really short imo. |
…-python into reindl-components
7 participants
Closes #xxxxdocs/sphinx/source/referencefor API changes.docs/sphinx/source/whatsnewfor 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`).remote-data) and Milestone are assigned to the Pull Request and linked Issue.This PR is part of my GSoC 2026 project. One of the goals is to add support for
return_componentsto all diffuse transposition models inpvlib, which currently is supported by some models (e.g.perez) but not others. This parameter allows the user to choose whether they want only the total diffuse irradiance, or are interested in the split between different diffuse components (sky diffuse, circumsolar, horizon brightening).This first PR starts by implementing
return_componentsinreindl.Reindl expands the Hay and Davies model, which considers only the isotropic and circumsolar components, and is defined as:
with A Rb being the circumsolar component and the rest being the isotropic component.
Reindl expands this by adding a horizon brightening component, but this new component is not additive but rather multiplied by the existing isotropic component:
In my implementation, I essentially considered that the isotropic term was:
while the horizon brightening term was:
I updated the function docstrings accordingly. I haven't yet added a test. I saw that e.g.
haydavieshas two separate tests, forreturn_components=Falseandreturn_components=True. However, from what I gathered, the test values came from the matlab implementation ofpvlib, and therefore it seems kind of silly to create a test using values derived from the python implementation to test the python implementation. Curious as to what are the community's thoughts on this.