Conversation
With v_oc = 0 the bracket passed to find_minimum collapsed to (-1, 0, 0), breaking its x1 < x2 < x3 requirement. Where v_oc is 0, use 1 as the right end: current falls with voltage, so the bracket still holds the minimum at 0. Closes pvlib#2671 Assisted-by: Claude Opus 5
Hey @Blizzeq! 🎉Thanks for opening your first pull request! We appreciate your If AI is used for any portion of this PR, you must vet the content |
|
I don't agree with enlarging the interval by assigning a positive value to the right endpoint. That risks returning a small
|
Keeps the find_minimum bracket valid without letting v_mp exceed v_oc. Assisted-by: Claude Opus 5.5
|
Agreed, thanks. You said the same in the issue and I missed it. I measured it: with the positive right end, v_mp came back as 9.9e-17 at v_oc = 0. The bracket I pushed a change that sets v_mp and p_mp to exactly 0 wherever v_oc is 0, since 0 is the only voltage in [0, v_oc]. Those elements still get a placeholder bracket so scipy sees x1 < x2 < x3, but its result is discarded. The test now asserts v_mp == 0 exactly. |
v0.16.0 is released, so the entry for pvlib#2671 belongs to the next version. Assisted-by: Claude Opus 5.5
|
Merged main and moved the whatsnew entry to v0.16.2, since v0.16.0 is out. |
| # left negative to insure strict inequality. Where v_oc is 0 the | ||
| # bracket would collapse to x2 == x3 (GH 2671), and 0 is the only | ||
| # voltage in [0, v_oc], so v_mp and p_mp are 0 there. Those elements | ||
| # get a placeholder bracket whose result is discarded. |
There was a problem hiding this comment.
| # get a placeholder bracket whose result is discarded. | |
| # Set up initial value x1 < x2 < x3 to find vmp. | |
| # Set x2 to v_oc. Set x1=-1, negative to insure strict inequality | |
| # x1 < x2. Set right to 1.0 where v_oc =0 to ensure | |
| # x2 < x3, otherwise set x3 = np.abs(v_oc). |
There was a problem hiding this comment.
Applied, with x2 written as 0.8*v_oc and two lines about the override below.
| # voltage in [0, v_oc], so v_mp and p_mp are 0 there. Those elements | ||
| # get a placeholder bracket whose result is discarded. | ||
| voc_zero = v_oc == 0 | ||
| init = (-1., 0.8*v_oc, np.where(voc_zero, 1., v_oc)) |
There was a problem hiding this comment.
| init = (-1., 0.8*v_oc, np.where(voc_zero, 1., v_oc)) | |
| init = (-1., 0.8*v_oc, np.where(voc_zero, 1., np.abs(v_oc))) |
There was a problem hiding this comment.
Applied. One side effect I measured: for v_oc < -1.25 the bracket is no longer monotonic, so find_minimum returns NaN, while main returns a negative v_mp there (v_oc = -50 gives v_mp = -25). For -1.25 < v_oc < 0 it is the other way round: NaN on main, a value with np.abs. Tell me if you want to keep the old result for large negative v_oc.
There was a problem hiding this comment.
I'm not going to argue with your AI about this. V_oc < -1.25 clearly indicates a failure upstream somewhere. Its not a relevant condition to handle here.
| v_mp = np.where(voc_zero, 0., res.x)[()] | ||
| p_mp = np.where(voc_zero, 0., -1.*res.f_x)[()] |
There was a problem hiding this comment.
Are these necessary? If they are an attempt to clean up output for a user, I'd prefer to preserve negative v_mp, p_mp if only to detect numerical problems, or problems with input parameters.
There was a problem hiding this comment.
They only apply where v_oc == 0 exactly, so negative v_mp or p_mp from other inputs pass through unchanged. I added them after your comment from Sep 22: with x3 = 1 the minimizer returns v_mp = 9.9e-17 at v_oc = 0, which is above v_oc. p_mp is already 0 there. I can drop both and assert v_mp close to 0 in the test if you prefer the raw value.
2 participants
RuntimeWarning: invalid value encountered in dividewhen usingsinglediode()#2671docs/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.With
v_oc = 0the bracket passed tofind_minimumwas(-1, 0, 0), which breaks thex1 < x2 < x3requirement @jwhitaker-gridcog pointed at, and scipy warns from a 0/0 inside the solver. Instead of silencing the warning, the right end of the bracket is 1 whereverv_ocis 0. Current falls with voltage and is zero atv_oc, so-v*I(v)is positive on both sides of 0 and any positive right end still brackets the minimum there.Results are unchanged wherever
v_oc > 0. Atv_oc = 0you still getv_mpof about 0 andp_mp = 0, now without the warning. The new test turns warnings into errors and fails on main; it skips on scipy older than 1.15, which takes the golden section path.