Skip to content

Clarify which SAPM parameters are required by module in pvsystem.sapm#2435

Merged
kandersolar merged 11 commits into
pvlib:mainfrom
RDaxini:pvsystem_sapm_table
Apr 23, 2025
Merged

Clarify which SAPM parameters are required by module in pvsystem.sapm#2435
kandersolar merged 11 commits into
pvlib:mainfrom
RDaxini:pvsystem_sapm_table

Conversation

@RDaxini

@RDaxini RDaxini commented Apr 10, 2025

Copy link
Copy Markdown
Member
  • Closes pvsystem.sapm documentation: clarification of coefficients used #2392
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries in docs/sphinx/source/reference for API changes.
  • Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for 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`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

@RDaxini RDaxini added this to the v0.12.1 milestone Apr 10, 2025
@RDaxini RDaxini changed the title Clarify which SAPM parameters are required in module argument of pvsystem.sapm Clarify which SAPM parameters are required by module in pvsystem.sapm Apr 10, 2025
@RDaxini RDaxini marked this pull request as ready for review April 10, 2025 22:50
Comment thread pvlib/pvsystem.py Outdated
Comment thread docs/sphinx/source/whatsnew/v0.12.1.rst Outdated
Comment thread docs/sphinx/source/whatsnew/v0.12.1.rst Outdated
Comment thread pvlib/pvsystem.py Outdated
Comment thread pvlib/pvsystem.py Outdated
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>

@echedey-ls echedey-ls left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. While this PR does address the linked issue, I find the optimal solution would also be to document in the effective_irradiance parameter, 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.

Comment thread pvlib/pvsystem.py Outdated
@RDaxini

RDaxini commented Apr 22, 2025

Copy link
Copy Markdown
Member Author
  1. 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.

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.

@cwhanse

cwhanse commented Apr 22, 2025

Copy link
Copy Markdown
Member

@RDaxini you didn't. I prefer the "these are included" version, but I was avoiding recommending a big edit :)

@kandersolar kandersolar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @RDaxini!

@kandersolar kandersolar merged commit 216386e into pvlib:main Apr 23, 2025
@RDaxini RDaxini deleted the pvsystem_sapm_table branch April 23, 2025 20:49
@kandersolar kandersolar modified the milestones: v0.12.1, v0.13.0 Jun 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pvsystem.sapm documentation: clarification of coefficients used

4 participants