Add scipy.optimize.elementwise.find_root (method='chandrupatla') to bishop88 functions
#2498
Conversation
| @@ -162,12 +162,11 @@ def bishop88(diode_voltage, photocurrent, saturation_current, | |||
| # calculate temporary values to simplify calculations | |||
| v_star = diode_voltage / nNsVth # non-dimensional diode voltage | |||
| g_sh = 1.0 / resistance_shunt # conductance | |||
| if breakdown_factor > 0: # reverse bias is considered | |||
There was a problem hiding this comment.
Had to get rid of the if for vectorized evaluation in find_root. The math works out the same either way, it's just a bit of unnecessary computation when breakdown_factor is zero.
(see also #1821)
There was a problem hiding this comment.
Not sure now why that was there, maybe thinking to save few microseconds.
| @@ -177,18 +176,14 @@ def bishop88(diode_voltage, photocurrent, saturation_current, | |||
| grad_i_recomb = np.where(is_recomb, i_recomb / v_recomb, 0) | |||
| grad_2i_recomb = np.where(is_recomb, 2 * grad_i_recomb / v_recomb, 0) | |||
| g_diode = saturation_current * np.exp(v_star) / nNsVth # conductance | |||
| if breakdown_factor > 0: # reverse bias is considered | |||
|
I think Chandrupatla is a great alternative to Brent Q or Toms-748, neither which are vectorized. The Cythonized version of Brent Q is problematic to deploy in pvlib, and Newton can fail if guesses are too far from the roots. Another option to consider is Toms-748 with Numba |
Feel free to open an issue regarding that! It's a bit out-of-scope for this PR IMO (not the issue, as long as it's timing gets compared there). It would make for a great contribution thou. |
pvlib/singlediode.py
Outdated
| Either ``'newton'``, ``'brentq'``, or ``'chandrupatla'`` (requires | ||
| scipy 1.15 or greater). ''method'' must be ``'newton'`` |
There was a problem hiding this comment.
At this point I would send users to See methods allowed by :py:func:`pvlib.singlediode.bishop88_mpp` or equivalent.
Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
|
Coverage failure should be ignored. It is failing because coverage is calculated based on the python 3.9 jobs, and these tests are skipped on python 3.9 because of the required scipy version. |
| @@ -162,12 +162,11 @@ def bishop88(diode_voltage, photocurrent, saturation_current, | |||
| # calculate temporary values to simplify calculations | |||
| v_star = diode_voltage / nNsVth # non-dimensional diode voltage | |||
| g_sh = 1.0 / resistance_shunt # conductance | |||
| if breakdown_factor > 0: # reverse bias is considered | |||
There was a problem hiding this comment.
Not sure now why that was there, maybe thinking to save few microseconds.
| Either ``'newton'`` or ``'brentq'``. ''method'' must be ``'newton'`` | ||
| if ``breakdown_factor`` is not 0. | ||
| Either ``'newton'``, ``'brentq'``, or ``'chandrupatla'``. | ||
| ''method'' must be ``'newton'`` if ``breakdown_factor`` is not 0. |
There was a problem hiding this comment.
Why this requirement for 'newton'? The arguments passed when method='brentq' or method='chandrupatla' include all the reverse bias parameters.
There was a problem hiding this comment.
I do not know the answer to this. I see in #948 you wrote this:
The Lambert W technique cannot solve the single diode equation with the term for current at reverse bias. The Bishop '88 methods can solve the equation. Tested with method='newton', not clear if method='brentq' is reliable for this application.
There was a problem hiding this comment.
My apologies for asking you to explain something I added 5 years ago :). I didn't pick up that all you did was reformat a bit of documentation.
I think that comment was added because the tests didn't confirm that 'brentq' works. I can't think of a reason why it wouldn't work. I suspect there was a vectorization issue that I didn't see how to fix at the time. It may be gone with removal of the if breakdown > 0: statement.
| @pytest.mark.parametrize('method', ['lambertw', 'brentq', 'newton', | ||
| chandrupatla]) | ||
| def test_i_from_v_size(method): | ||
| if method == 'newton': |
There was a problem hiding this comment.
Is this why the previous restriction to method='newton'?
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
optimize.elementwisefunctions #2497[ ] Updates 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.This PR adds
method='chandrupatla'to the bishop88 functions. I decided to leavelambertwfor a separate PR, since it's not clear how to opt into using the new scipy function instead of our golden section search.Some basic timing comparisons for the wrapper functions in
pvlib.pvsystem(all times in milliseconds):singlediodev_from_ii_from_vStill need to figure out a clean way to skip the tests on python 3.9, where this new functionality is not available.