Skip to content

Villalva Algorithm Pull Request - #2878

Open
JFrederico2022 wants to merge 42 commits into
pvlib:mainfrom
JFrederico2022:main
Open

JFrederico2022 wants to merge 42 commits into
pvlib:mainfrom
JFrederico2022:main

Conversation

@JFrederico2022

@JFrederico2022 JFrederico2022 commented Oct 3, 2026 •

Copy link
Copy Markdown
  • Closes #xxxx
  • I am familiar with the contributing guidelines
  • I attest that all AI-generated material has been vetted for accuracy and is in compliance with the pvlib license
  • 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.

@github-actions

github-actions Bot commented Oct 3, 2026

Copy link
Copy Markdown

Hey @JFrederico2022! 🎉

Thanks for opening your first pull request! We appreciate your
contribution. Please ensure you have reviewed and understood the
contributing guidelines.

If AI is used for any portion of this PR, you must vet the content
for technical accuracy.

Finally, be sure the PR description includes the PR
checklist,
and complete the items you are able to.

@cwhanse

cwhanse commented Oct 4, 2026

Copy link
Copy Markdown
Member

@JFrederico2022 pvlib should get only the functions, not the notebook. It has been a while since we discussed that: #2754 (comment) "In pvlib, a calcparams_villalva function would go into pvlib.pvsystem. Code for fitting would go into pvlib.ivtools.sdm in a new module villalva.py"

If you want, you can convert the notebook to a python script and add to the Example Gallery. Just start the script name with "plot_" and use docstrings where you want text to appear.

@JFrederico2022

Copy link
Copy Markdown
Author

Hello, @cwhanse

I updated the docs. Please, let me know if they are all right.

Best regards,
João Frederico

Comment thread pvlib/ivtools/sdm/villalva.py Outdated
Comment thread pvlib/ivtools/sdm/villalva.py Outdated
Comment thread pvlib/ivtools/sdm/villalva.py Outdated
Comment thread pvlib/pvsystem.py Outdated
JFrederico2022 and others added 3 commits October 6, 2026 21:09
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
@ramaroesilva

Copy link
Copy Markdown
Contributor

Hi @JFrederico2022, great initiative!

In my opinion, I think it would be nice for your PR to have a corresponding issue which includes a description of what you are adding to pvlib (e.g., what do you see is missing that a PR would add and include sources). That way reviewers and curious people can have a quick outlook of what's happening.

@RDaxini

RDaxini commented Oct 7, 2026 •

Copy link
Copy Markdown
Member

Hi @JFrederico2022, great initiative!

In my opinion, I think it would be nice for your PR to have a corresponding issue which includes a description of what you are adding to pvlib (e.g., what do you see is missing that a PR would add and include sources). That way reviewers and curious people can have a quick outlook of what's happening.

The issue exists (#2754) but, @JFrederico2022, please tag this in the PR template

Comment thread pvlib/ivtools/sdm/villalva.py Outdated
Comment thread pvlib/ivtools/sdm/villalva.py Outdated
@echedey-ls

Copy link
Copy Markdown
Member

Remember to list the new functions in https://github.com/pvlib/pvlib-python/blob/main/docs/sphinx/source/reference/pv_modeling/sdm.rst?plain=1

@cwhanse cwhanse added this to the v0.16.2 milestone Oct 7, 2026

@RDaxini RDaxini 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.

I have suggested a few basic docstring revisions. You can implement changes recommended by reviewers either by committing them individually or, under the Files changed tab, adding the suggestions to a batch and committing them all at once.

Happy to look more in depth at the core implementation in the coming days

Comment thread pvlib/ivtools/sdm/villalva.py Outdated
Comment thread pvlib/ivtools/sdm/villalva.py Outdated
Comment thread pvlib/ivtools/sdm/villalva.py Outdated
Comment thread pvlib/ivtools/sdm/villalva.py Outdated
Comment thread pvlib/ivtools/sdm/villalva.py
Comment thread pvlib/pvsystem.py
Comment thread pvlib/pvsystem.py Outdated
Comment thread docs/examples/iv-modeling/plot_villalva.py Outdated
Comment thread pvlib/ivtools/sdm/__init__.py
JFrederico2022 and others added 7 commits October 9, 2026 09:11
Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
Co-authored-by: Rajiv Daxini <143435106+RDaxini@users.noreply.github.com>
Co-authored-by: Rajiv Daxini <143435106+RDaxini@users.noreply.github.com>
Co-authored-by: Rajiv Daxini <143435106+RDaxini@users.noreply.github.com>
Co-authored-by: Rajiv Daxini <143435106+RDaxini@users.noreply.github.com>
Co-authored-by: Rajiv Daxini <143435106+RDaxini@users.noreply.github.com>
JFrederico2022 and others added 10 commits October 9, 2026 09:17
Co-authored-by: Rajiv Daxini <143435106+RDaxini@users.noreply.github.com>
Co-authored-by: Rajiv Daxini <143435106+RDaxini@users.noreply.github.com>
Co-authored-by: Rajiv Daxini <143435106+RDaxini@users.noreply.github.com>
Updated references for M. G. Villalva's work in the documentation.
Added noqa comments to suppress E231 warnings for print statements and labels.
Added fit_villalva function to the list of fitting functions.
@markcampanelli

markcampanelli commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

@JFrederico2022 For the validation plot below, my understanding is that the Villalva model was fit to each temperature and irradiance condition separately. However, as I understand it, the original SDM paper describes using the Isc and Voc temperature coefficients to compute I0 away from STC (in particular, avoiding the need for a band gap parameter value). I am curious what the validation looks like using that extrapolation of the fit at STC, rather than separate "local" fits at different operating conditions. For PVfit, I have used IEC 61853-1 matrix measurements to assess how well a fit using STC conditions extrapolates to a wider set of conditions, and I have attached an example of that. (IIRC, this was a rather good fit, but other fits do not extrapolate so well away from STC.)

More pertinent to this PR, is the SDM using the Isc and Voc temperature coefficients implemented demonstrated here?

image image

Comment thread pvlib/pvsystem.py Outdated

@cwhanse cwhanse 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.

@JFrederico2022 any progress towards some unit tests for these new functions?

Comment thread pvlib/ivtools/sdm/villalva.py Outdated
Comment thread pvlib/ivtools/sdm/villalva.py Outdated
Comment thread pvlib/ivtools/sdm/villalva.py Outdated
Comment thread pvlib/ivtools/sdm/villalva.py Outdated
Comment thread pvlib/ivtools/sdm/villalva.py Outdated
Comment thread pvlib/pvsystem.py Outdated
Comment thread pvlib/pvsystem.py Outdated
Comment thread pvlib/pvsystem.py
Comment thread pvlib/pvsystem.py Outdated
Comment thread pvlib/pvsystem.py
Comment on lines +1709 to +1713
The finite-:math:`R_{sh}` open-circuit relation is used for the
temperature-dependent saturation current so that the operating-condition
equations remain consistent with the fitted reference parameters. The
fitted :math:`R_s` and :math:`R_{sh}` are held constant with operating
condition, following the Villalva modeling assumption used here.

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.

This short paragraph leaves me wanting to see the equations that make up the model.

JFrederico2022 and others added 9 commits October 21, 2026 14:23
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Comment thread pvlib/ivtools/sdm/villalva.py Outdated
Comment thread pvlib/pvsystem.py Outdated
JFrederico2022 and others added 6 commits October 22, 2026 17:07
Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
Co-authored-by: Rajiv Daxini <143435106+RDaxini@users.noreply.github.com>
Updated mathematical expressions and descriptions in the Villalva SDM parameters calculation for clarity.
Added test for calcparams_villalva to check operating-condition results and NaN propagation.
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.

7 participants