pvlib
Fix2461
Enhance Huld model with EU JRC coefficients
- 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.
I'm -1 on the new kwarg use_eu_jrc. The way I read the 2025 reference, the intent is to replace the older set of coefficients, to reflect "modern" modules. If we need to retain the older coefficients for continuity, we can add a table to the docstring.
Comments for my fellow maintainers?
I'm between @cwhanse and @OmarBahamida ; I prefer a version keyword only parameter that can be either 2025 or 2011, and defaults to the latest one. I think it's valuable to ease the use of older parameters, given their lifetime is long and post analysis of systems is great for diagnosis.
I'm between @cwhanse and @OmarBahamida ; I prefer a
versionkeyword only parameter that can be either2025or2011, and defaults to the latest one. I think it's valuable to ease the use of older parameters, given their lifetime is long and post analysis of systems is great for diagnosis.
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 version parameter is not specified. Eventually, we can make the breaking change of switching the default version to the most recent version of the coefficients.
Any set of coefficients can be provided directly to the existing function, so kwargs like version aren't strictly necessary.
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: version=None would have my vote.
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.
Hi all, I’ve just pushed changes to fix the Flake8 linter errors (trailing whitespace and long lines). I also verified that all new code is fully covered by tests (100% coverage for pvlib/pvarray.py). Sorry for my late response, I haven’t had enough time recently, but I appreciate your patience. Thank you for your feedback and support.
@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.
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.
