Clarify which SAPM parameters are required by module in pvsystem.sapm
#2435
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RDaxini
changed the title
module argument of pvsystem.sapmmodule in pvsystem.sapm
cwhanse
reviewed
Apr 10, 2025
echedey-ls
reviewed
Apr 11, 2025
cwhanse
approved these changes
Apr 16, 2025
Co-authored-by: Cliff Hansen <[email protected]>
echedey-ls
reviewed
Apr 18, 2025
There was a problem hiding this comment.
The following module keys are being used in pvlib.pvsystem.sapm, module parameter (do not match with current text):
Ordering by when they appear in code
Bvmpo
Mbvmp
Bvoco
Mbvoc
N
Cells_in_Series
Isco
Aisc
Impo
C0
C1
Aimp
Voco
Vmpo
C2
C3
IXO
C4
C5
Aisc
IXXO
C6
C7
Aimp
Ordering same as table
C0-C7
Isco
Impo
Voco
Vmpo
Aisc
Aimp
Bvoco
Mbvoc
Bvmpo
Mbvmp
N
Cells_in_Series
IXO
IXXO
Two points:
- It's usually better to speak/reflect statements in positive, so no ambiguities are left and in this case you also save time to the end user by not getting the full list, then excluding the list of unused parameters.
- While this PR does address the linked issue, I find the optimal solution would also be to document in the
effective_irradianceparameter, that the effective irradiance is calculated by using the other sapm function(s) (which right now are not listed in the See Also section, adding them too would be a nice improvement)
EDIT: I see this review does not partially apply due to PR #2433, I did not see it earlier.
Ah did I misunderstand @cwhanse's earlier suggestion? I switched from listing which are included (although that list was incomplete in that commit) to listing which are not included. |
|
@RDaxini you didn't. I prefer the "these are included" version, but I was avoiding recommending a big edit :) |
echedey-ls
approved these changes
Apr 23, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
pvsystem.sapmdocumentation: clarification of coefficients used #2392Tests addedUpdates entries indocs/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.