Skip to content

Conversation

@kandersolar
Copy link
Member

@kandersolar kandersolar commented Oct 1, 2025

  • Closes Datasheet-only nonlinear model for Pmp/Imp/Vmp/Isc/Voc #2562
  • 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.

@kandersolar kandersolar added this to the v0.13.2 milestone Oct 2, 2025
@kandersolar kandersolar marked this pull request as ready for review October 2, 2025 18:01
@kandersolar
Copy link
Member Author

This PR currently adds three new functions. As they are related to each other I thought it would be helpful to review them together, but I can split the PR into smaller pieces to make it more digestible if needed.

To summarize the three functions:

  • pvlib.ivtools.sdm.fit_desoto_batzelis: estimate De Soto SDM parameters from datasheet values
  • pvlib.singlediode.batzelis_keypoints: estimate Pmp/Imp/Vmp/Isc/Voc from SDE parameters
  • pvlib.pvarray.batzelis: estimate Pmp/Imp/Vmp/Isc/Voc from datasheet values

The third, effectively, combines the first two with calcparams_desoto, but with some algebraic manipulations to make the computation more compact. I think there is also a minor difference in the calcparams_desoto calculations. All three come from the paper linked in #2562.

Questions for reviewers:

  • Currently alpha_sc has units [1/K] in some places and [A/K] in others. Better to diverge from the reference (which uses [1/K]) and use [A/K] everywhere for consistency with the rest of pvlib?
  • Parameter names likely also should be changed for consistency. i_sc instead of isc0, etc?
  • Are the current modules the best places for these new functions?

Copy link
Member

@cwhanse cwhanse left a comment

Choose a reason for hiding this comment

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

Some reactions.

@cwhanse
Copy link
Member

cwhanse commented Oct 2, 2025

A question about the method: does batzelis produce the same results as fit_desoto_batzelis + batzelis_keypoints? It is not clear to me that should be the case.

@kandersolar
Copy link
Member Author

A question about the method: does batzelis produce the same results as fit_desoto_batzelis + batzelis_keypoints? It is not clear to me that should be the case.

Close but not exactly the same, in my testing. The latter has to be fit_desoto_batzelis + calcparams_desoto + batzelis_keypoints, and I think there is at least a difference in the calcparams_desoto step (regarding dEgdT). I think there are differences elsewhere too, but I can't point to any specifics.

@adriesse
Copy link
Member

adriesse commented Oct 30, 2025

* Currently `alpha_sc` has units [1/K] in some places and [A/K] in others.  Better to diverge from the reference (which uses [1/K]) and use [A/K] everywhere for consistency with the rest of pvlib?

I think this is the right decision for pvlib because of precedent, although personally I much prefer the normalized values.

* Parameter names likely also should be changed for consistency.  `i_sc` instead of `isc0`, etc?

+1

* Are the current modules the best places for these new functions?

+1

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.

Datasheet-only nonlinear model for Pmp/Imp/Vmp/Isc/Voc

4 participants