Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add mismatch.py and function to combine curves #1781

Draft
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

ajonesr
Copy link
Contributor

@ajonesr ajonesr commented Jun 20, 2023

  • 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.

Adds a function combine_curves that combines curves (in series) and a function that prepares inputs for combine_curves. This is part of the work to move some of the functionality of PVMismatch over to pvlib.

pvlib/ivtools/mismatch.py Outdated Show resolved Hide resolved
pvlib/ivtools/mismatch.py Outdated Show resolved Hide resolved
pvlib/ivtools/mismatch.py Outdated Show resolved Hide resolved
@ajonesr ajonesr marked this pull request as ready for review June 23, 2023 19:31
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.

Just need to add two tests that raise the ValueErrors.

pvlib/ivtools/mismatch.py Outdated Show resolved Hide resolved
pvlib/ivtools/mismatch.py Outdated Show resolved Hide resolved
pvlib/ivtools/mismatch.py Outdated Show resolved Hide resolved
pvlib/tests/ivtools/test_mismatch.py Outdated Show resolved Hide resolved
pvlib/tests/ivtools/test_mismatch.py Outdated Show resolved Hide resolved
ajonesr and others added 8 commits June 26, 2023 14:13
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>
@cwhanse cwhanse added this to the 0.10.1 milestone Jun 28, 2023
@cwhanse
Copy link
Member

cwhanse commented Jun 28, 2023

@pvlib/pvlib-maintainer would appreciate your look at this, when you can.

@AdamRJensen
Copy link
Member

Would it be possible to add a simple example to the documentation?

@kandersolar kandersolar modified the milestones: 0.10.1, v0.10.2 Jul 6, 2023
@ajonesr
Copy link
Contributor Author

ajonesr commented Jul 10, 2023

@AdamRJensen I've added an example in docs/examples/iv-modeling, though I'm not sure that's the right place to put it. Let me know if I should move it somewhere else

@AdamRJensen
Copy link
Member

AdamRJensen commented Jul 13, 2023

@AdamRJensen I've added an example in docs/examples/iv-modeling, though I'm not sure that's the right place to put it. Let me know if I should move it somewhere else

Thanks a lot @ajonesr !
Could you add a legend for the individual lines, e.g., just panel 1, panel 2, and panel 3. Also, if you could add just a tiny bit more text in the example about what is going on and why it is useful :)

You also need to move the whatsnew entry from v0.10.0 to v0.10.2.

@ajonesr
Copy link
Contributor Author

ajonesr commented Jul 14, 2023

@AdamRJensen I've added an example in docs/examples/iv-modeling, though I'm not sure that's the right place to put it. Let me know if I should move it somewhere else

Thanks a lot @ajonesr ! Could you add a legend for the individual lines, e.g., just panel 1, panel 2, and panel 3. Also, if you could add just a tiny bit more text in the example about what is going on and why it is useful :)

You also need to move the whatsnew entry from v0.10.0 to v0.10.2.

Sure thing! Just pushed those changes.

@kandersolar kandersolar self-requested a review July 19, 2023 15:48
@kandersolar
Copy link
Member

I can do a review (sorry for the wait!) but I'm a little hesitant about the current approach this PR takes. To me, this code feels more like an application than it does reusable functions. For example, combine_curves is pretty specific in its requirements, which makes it suitable for use with prepare_curves, but difficult to use if the curves come from anywhere else.

I wonder if these functions would make more sense as a gallery example page than functions in pvlib itself?

@cwhanse
Copy link
Member

cwhanse commented Jul 21, 2023

@kandersolar I'm interested to hear your thoughts on different ways to organize this capability.

I don't think a gallery example is enough here. We've heard several requests for capabilities native to pvlib to model mismatch. It is clear that pvmismatch, while useful, has some limits and is not going to be further developed. It seems reasonable to include functions in pvlib that one could use to get beyond the limits of pvmismatch.

Curves are added by aligning current and summing the corresponding voltage. The functions in this PR meet one use case: I have diode equation parameters for the series-connected modules/cells and I want the string-level IV curve.I can see a second use case: I have non-aligned IV curves (measured perhaps) and want the string-level curve. These functions don't meet the measured-curve use case.

I think separate function(s) that align the currents are useful, so that the function which adds curves is more reusable. We would want to align currents starting either from diode equation values (prepare_curves does this now) and from curve data itself (by interpolation, likely with extrapolation to negative voltages).

Maybe combine_curves should be add_curves. That would free the more general combine_curves name for the measured-curve use case, combine_curves(current_array, voltage_array). In that context, combine_curves would handle interpolation to align currents, then call add_curves.

@kandersolar
Copy link
Member

I think my hesitation was centered around not being able to use the addition function with the existing methods for generating IV curves. It felt somewhat disconnected from the rest of pvlib. A function to align curves on one of the two dimensions would address that.

The tight coupling of prepare_curves to the SDE makes me wonder if it should live in pvlib.singlediode as a new bishop88_* function.

Series vs parallel connection is something else to consider when choosing names to leave room for future functionality.

@adriesse
Copy link
Member

I don't think a gallery example is enough here.

It could be a start though...

@cwhanse
Copy link
Member

cwhanse commented Aug 9, 2023

Documenting an offline conversation about this PR. One concern is that the draft prepare_curves function contains a new model for reverse bias current that ought to live in singlediode. Another concern is that only one of two use cases are addressed:

"I have diode model parameters and want a series-combined IV curve."

The other use case is

"I have IV curves and want to combine them in series"

This second use case doesn't assume that the IV curves in hand are aligned to common currents, a job which prepare_curves is doing for modeled curves.

We propose the following next steps:

  1. Move the reverse bias current calculation out of pvlib.singlediode.bishop88 to a helper function, tentatively named _reverse_bias_exp
  2. Add a second reverse bias current model to pvlib.singlediode, tentatively named _reverse_bias_simple, which models the negative voltage portion of the IV curve with the single diode equation (without the diode breakdown current term from bishop88) and a vertical asymptote at the breakdown voltage.
  3. Add a kwarg to the bishop88 functions to select either the exponential or simple model for reverse bias current. This kwarg would replace the overloading of breakdown_factor as a flag to turn on the reverse bias current term.
  4. Rework the prepare_curves function to use these revised bishop88 functions. prepare_curves accepts diode model parameters and returns curves aligned to common currents.
  5. Add a new function align_curves that accepts IV curves and returns those curves aligned to common currents. This function will use an interpolator, extrapolate to negative voltage and use the simple reverse bias current model, as necessary.

@ajonesr ajonesr marked this pull request as draft August 9, 2023 21:21
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.

None yet

5 participants