Skip to content

Conversation

cwhanse
Copy link
Member

@cwhanse cwhanse commented Oct 10, 2025

  • Closes Use scipy's new optimize.elementwise functions #2497
  • 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.

Uses 'chandrupatla' when scipy>=1.15. Existing tests are good enough.

@cwhanse cwhanse added this to the v0.13.2 milestone Oct 10, 2025
@cwhanse
Copy link
Member Author

cwhanse commented Oct 10, 2025

Restart of #2567

@cwhanse
Copy link
Member Author

cwhanse commented Oct 10, 2025

Pursuing the test failures, one condition tested is photocurrent=0. For that input, i_mp=nan is returned when the scipy method is used (#2567 (comment)).

nan is returned because the convergence fails on the first iteration, due to violation of the bracket condition. Quote from the scipy documentation for find_minimum:

requires argument init to provide a three-point minimization bracket: x1 < x2 < x3 such that func(x1) >= func(x2) <= func(x3), where one of the inequalities is strict.

Here, init = (0., 0.8*v_oc, v_oc) and func=_vmp_opt in the PR.

Evaluating at photocurrent=0. and v_oc=init, I get _vmp_opt(init, 0., ...) = array([-0.00000000e+00, 2.18952885e-48, 2.73691106e-48]). Note that the negative power is actually positive. The func values at this bracket fail the check.

Digging deeper, _lambertw_i_from_v(init, 0., ...) = array([0., -4.1359e-25, -4.1359e-25]) the negative currents are the root of the problem.

We could set small negative current to zero in lambertw_i_from_v But the MPP with chandralupta would still fail since photocurrent=0. since func(init) would be array([-0., -0., -0.]) and one of the inequalities has to be strict.

I think intercepting small photocurrent and skipping the MPP (or entire IV curve) calculation is better.

Your thoughts?

@kandersolar
Copy link
Member

I think intercepting small photocurrent and skipping the MPP (or entire IV curve) calculation is better.

I think this is probably fine, although the indexing is going to make the code clunky.

A possible alternative: instead of init = (0., 0.8*v_oc, v_oc), how about init = (-1, 0.8*v_oc, v_oc)? For photocurrent=0, the MPP is at v=i=0, and if we're not satisfying the bracket condition for the right pair, how about moving the left side out so that it's satisfied by the left pair?

@kandersolar kandersolar changed the title Use chandralupta to find MPP in lambertw method Use chandrupatla to find MPP in lambertw method Oct 15, 2025
@cwhanse
Copy link
Member Author

cwhanse commented Oct 15, 2025

init = (-1, 0.8*v_oc, v_oc)?

This is the way. And reverting from i_mp = p_mp / v_mp which introduces a divide by 0. and a nan thus a test failure...

@cwhanse
Copy link
Member Author

cwhanse commented Oct 15, 2025

Weird test failure seemingly unrelated to these code changes.

@kandersolar
Copy link
Member

Weird test failure seemingly unrelated to these code changes.

I think it is related. The failing test has negative v_mp values (e.g. -5e-16). Presumably that wasn't possible when the MPP search was bounded by zero.

Would truncating negative v_mp to zero after the MPP search make sense? Not sure what is best here.

Co-authored-by: Kevin Anderson <kevin.anderso@gmail.com>
@cwhanse
Copy link
Member Author

cwhanse commented Oct 15, 2025

Thanks, I was missing how the test failure related to the changes in this PR. That fixture evaluates the CEC model via the model chain.

The failure exposed additional issues however:

  1. The test executes a modelchain which, for input weather and for inverter_model='adr' produces this output with numpy 2.2 and scipy 1.5.3:
weather
                           ghi  dni  dhi
2016-01-01 12:00:00-07:00  500  800  100
2016-01-01 18:00:00-07:00    0    0    0

mc.results.dc[['v_mp', 'p_mp']]
                                   v_mp          p_mp
2016-01-01 12:00:00-07:00  4.062850e+01  1.679352e+02
2016-01-01 18:00:00-07:00 -5.205590e-16  2.691227e-41

mc.results.ac
2016-01-01 12:00:00-07:00     NaN
2016-01-01 18:00:00-07:00     NaN

The test fails because of the NaN at the second time step in the AC series:

    assert mc.results.ac.iloc[1] < 1

Apparently, prior to numpy 2.X, the assert was OK (the ubuntu conda min test passes). For the life of me I can't see why this test wasn't failing on the last few PRs that also use numpy 2.X.

The NaNs are introduced in the AC series in inverter.adr at this line:

power_ac = np.where(invalid, np.nan, power_ac)

Upstream, there are numeric values in power_ac.

Why is a NaN also in the first line, where the DC values look reasonable? Because the test system is malformed. The test system pairs a single 220W module with a 3kW inverter.

I think the fixes here is to repair the test: replace the above assert so that it handles NaN, and correct this fixture so that a reasonable AC power is output for the first time step:

def cec_dc_adr_ac_system(sam_data, cec_module_cs5p_220m,

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.

Use scipy's new optimize.elementwise functions

2 participants