Repository navigation
Conversation
Hey @mokazemi! 🎉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 |
c1c521f to
d6070ff
Compare
echedey-ls
left a comment
echedey-ls
left a comment
There was a problem hiding this comment.
There are other changes that must be done to to cec.py:79
- Update optional dependency from 'nrel-pysam' to 'nlr-pysam' - Uncomment nlr-pysam in Python 3.14 CI requirements - PySAM 8.0.0+ is distributed as nlr-pysam for Python 3.8-3.14 Note: test_fit_cec_sam shows minor numerical differences between nlr-pysam 8.0.0+ and nrel-pysam, particularly in the 'Adjust' parameter. This may indicate algorithmic changes in the newer nlr-pysam distribution that needs further investigation. Fixes pvlib#2871
d6070ff to
3c0187a
Compare
|
@mokazemi I believe @echedey-ls's comment still needs to be addressed: #2872 (review) |
Done. |
echedey-ls
left a comment
echedey-ls
left a comment
There was a problem hiding this comment.
@mokazemi I'm interested in knowing which AI toolchain you use. Are you on claude, codex, is the model any gpt, gemma, etc.? And are you using it for all commits or you only used it to kickstart the PR?
We will defer merging this PR until our CI is green once again. Meanwhile, feel free to list yourself in the relevant whatsnew file.
| Raises | ||
| ------ | ||
| ImportError | ||
| if NREL-PySAM is not installed. |
There was a problem hiding this comment.
| if ``NLR-PySAM`` is not installed. |
| raise ImportError("Requires NREL's PySAM package at " | ||
| "https://pypi.org/project/NREL-PySAM/.") | ||
| raise ImportError( | ||
| "Requires PySAM. Install 'nlr-pysam' for PySAM >= 8.0.0 " |
There was a problem hiding this comment.
| "Requires PySAM. Install 'nlr-pysam' for PySAM >= 8.0.0 " | |
| "Requires PySAM. Install 'NLR-PySAM' for PySAM >= 8.0.0 " |
package name resolution is case insensitive but let's try to adhere to the canonical name the authors gave to the package.
Changes
nrel-pysamtonlr-pysamnlr-pysamfor Python 3.8-3.14Testing
The existing test
test_fit_cec_samshows minor numerical differences when usingnlr-pysam8.0.0+:This may indicate algorithmic improvements or changes in nlr-pysam and should be reviewed by maintainers familiar with the PySAM library.
Closes #2871
nlr-pysampackage name #2871docs/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.