Add 2025 coefficients for Huld model#2486
Conversation
- Introduced a new function to infer updated coefficients for the Huld model based on EU JRC research. - Added a parameter to the existing Huld function to optionally use these updated coefficients. - Updated documentation to reflect the new functionality and included references to the EU JRC paper. - Added tests to verify the implementation and ensure compatibility with existing functionality. (cherry picked from commit f0ba338)
- Updated the test for the Huld model to use non-reference values for irradiance and temperature. - Enhanced the test to verify that results differ for all supported cell types when using EU JRC coefficients. - Added checks to ensure all cell types are supported and that a KeyError is raised for invalid cell types. (cherry picked from commit 7c0feba)
|
I'm -1 on the new kwarg Comments for my fellow maintainers? |
|
I'm between @cwhanse and @OmarBahamida ; I prefer a |
echedey-ls
left a comment
There was a problem hiding this comment.
Well done. Overall a good approach. Please, also add a whatsnew entry in docs/sphinx/source/whatsnew once you apply the new changes.
pvlib/pvarray.py
Outdated
| .. [2] EU JRC paper, "Updated coefficients for the Huld model", | ||
| https://doi.org/10.1002/pip.3926 |
There was a problem hiding this comment.
Looks like you didn't change this to fit the IEEE citation format nor the :doi: directive (see the citation just above).
I recommend you to read https://pvlib-python.readthedocs.io/en/latest/contributing/style_guide.html
tests/test_pvarray.py
Outdated
| res = pvarray.huld(1000, 25, 100) | ||
|
|
||
|
|
||
| def test_huld_eu_jrc(): |
There was a problem hiding this comment.
Can you check for some specific values, up to 4-5 decimals? It would be better if you calculate them outside of your Python implementation.
I fully support this implementation. It would be a shame to remove existing features. This way we can also have a deprecation cycle, i.e., a period where a warning is raised if the |
|
Any set of coefficients can be provided directly to the existing function, so kwargs like But if we lean towards providing built-in parameters, as a convenience, or as a means to repeat previous analyses (where parameters were not explicit, which is a different problem), then I think the user should be required to affirmatively select them: |
|
I think I lean towards @AdamRJensen 's suggestion, which maintains the default behaviour for now. |
|
@OmarBahamida any plan to move this forward? If not, I will do so. Thanks for starting this change. |
remove trailing whitespace and break long lines
|
Hi all, |
remove whitespace from blank lines and break long lines - Removed trailing whitespace from blank lines in pvlib/pvarray.py - Broke long lines in function definitions and assertions to comply with line length limits - Reformatted comments and code in tests/test_pvarray.py for Flake8 compliance
|
@OmarBahamida we'd like to get this change into the next release. I took the liberty of restructuring the new kwarg and the test that looks up the coefficients. |
|
@cwhanse Thank you very much for your help and for restructuring the code and tests! I’m glad to hear this will be included in the next release. I appreciate your feedback and support throughout this process. |
|
@pvlib/pvlib-maintainer this is ready for reviews |
Co-authored-by: Anton Driesse <[email protected]>
echedey-ls
left a comment
There was a problem hiding this comment.
Absolute nice tests! LGTM :)
|
Well, I guess at some point pvlib will have to change the default constants year, but that can be a problem for the future pvlib. |
Co-authored-by: Echedey Luis <[email protected]>
pvlib/pvarray.py
Outdated
| -0.001256, 0.000001), | ||
| 'cdte': (-0.046689, -0.072844, -0.002262, 0.000276, | ||
| 0.000159, -0.000006)} | ||
| if k_version == '2011': |
There was a problem hiding this comment.
I don't think '2011' is a good choice of identifier for the older coefficients. To me it suggests the 2011 paper is the source of those coefficients, which isn't the case.
I think something like PVGIS5.3 (the current version) and PVGIS{something} could be a good option, but I don't know what version will have the new coefficients. I've emailed Nikos Alexandris at JRC asking about this.
There was a problem hiding this comment.
Nikos responded that PVGIS 6 will use the new coefficients by default. Maybe PVGIS5 and PVGIS6 are good identifiers, then.
| Either ``'pvgis5'`` (default) or ``'pvgis6'``. Selects values | ||
| for ``k`` if ``k`` is not specified. If ``'pvgis5'``, values are | ||
| from PVGIS documentation and are labeled in [2]_ as "current". | ||
| If ``'pvgis6'`` values are from [2]_ labeled as "updated". |
There was a problem hiding this comment.
| Either ``'pvgis5'`` (default) or ``'pvgis6'``. Selects values | |
| for ``k`` if ``k`` is not specified. If ``'pvgis5'``, values are | |
| from PVGIS documentation and are labeled in [2]_ as "current". | |
| If ``'pvgis6'`` values are from [2]_ labeled as "updated". | |
| Selects a set of values for ``k`` if ``k`` is not specified: either ``'pvgis5'`` (default) | |
| or ``'pvgis6'``. For ``'pvgis5'``, ``k`` values are from PVGIS documentation, | |
| which are also identified as "current" in [2]_ . For ``'pvgis6'``, values are | |
| from [2]_ where they are labeled as "updated". |
There was a problem hiding this comment.
I'll not take this suggestion as the phrasing implies that a value of 'pvgis5' is assigned to k rather than being a value of k_version.
kandersolar
left a comment
There was a problem hiding this comment.
One typo to fix, otherwise LGTM
|
Thanks @OmarBahamida and all other contributors here! |
Enhance Huld model with EU JRC coefficients
Closes #2461