From 715e809bb308e4028ae8084ad90b25440c14844b Mon Sep 17 00:00:00 2001 From: Cliff Hansen Date: Wed, 7 Dec 2022 14:03:07 -0700 Subject: [PATCH 1/6] revise convergence, add test with equal upper and lower --- pvlib/tests/test_tools.py | 8 ++++++++ pvlib/tools.py | 27 ++++++++++++--------------- 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/pvlib/tests/test_tools.py b/pvlib/tests/test_tools.py index 167dca8cec..075c29968f 100644 --- a/pvlib/tests/test_tools.py +++ b/pvlib/tests/test_tools.py @@ -45,6 +45,14 @@ def test__golden_sect_DataFrame_vector(): v, x = tools._golden_sect_DataFrame(params, lower, upper, _obj_test_golden_sect) assert np.allclose(x, expected, atol=1e-8) + # upper and lower bounds equal + params = {'c': np.array([1., 2., 1.]), 'n': np.array([1., 1., 1.])} + lower = np.array([0., 0.001, 1.]) + upper = np.array([1., 1.2, 1.]) + expected = np.array([0.5, 0.25, 1.0]) # x values for maxima + v, x = tools._golden_sect_DataFrame(params, lower, upper, + _obj_test_golden_sect) + assert np.allclose(x, expected, atol=1e-8) def test__golden_sect_DataFrame_nans(): diff --git a/pvlib/tools.py b/pvlib/tools.py index 991568f9e0..8550904fbb 100644 --- a/pvlib/tools.py +++ b/pvlib/tools.py @@ -341,6 +341,8 @@ def _golden_sect_DataFrame(params, lower, upper, func, atol=1e-8): -------- pvlib.singlediode._pwr_optfcn """ + if np.any(upper - lower < 0.): + raise ValueError('upper > lower is required') phim1 = (np.sqrt(5) - 1) / 2 @@ -348,17 +350,15 @@ def _golden_sect_DataFrame(params, lower, upper, func, atol=1e-8): df['VH'] = upper df['VL'] = lower - converged = False - iterations = 0 # handle all NaN case gracefully with warnings.catch_warnings(): warnings.filterwarnings(action='ignore', message='All-NaN slice encountered') - iterlimit = 1 + np.nanmax( - np.trunc(np.log(atol / (df['VH'] - df['VL'])) / np.log(phim1))) + # handle upper == lower here + converged = np.nanmax(np.abs(upper - lower)) < atol - while not converged and (iterations <= iterlimit): + while not converged: phi = phim1 * (df['VH'] - df['VL']) df['V1'] = df['VL'] + phi @@ -373,19 +373,16 @@ def _golden_sect_DataFrame(params, lower, upper, func, atol=1e-8): err = abs(df['V2'] - df['V1']) - # works with single value because err is np.float64 - converged = (err[~np.isnan(err)] < atol).all() - # err will be less than atol before iterations hit the limit - # but just to be safe - iterations += 1 + converged = np.all(err[~np.isnan(err)] < atol) - if iterations > iterlimit: - raise Exception("Iterations exceeded maximum. Check that func", - " is not NaN in (lower, upper)") # pragma: no cover + # works with single value because err is np.float64 +# converged = (err[~np.isnan(err)] < atol).all() + # best estimate of location of maximum + df['max'] = 0.5 * (df['V1'] + df['V2']) try: - func_result = func(df, 'V1') - x = np.where(np.isnan(func_result), np.nan, df['V1']) + func_result = func(df, 'max') + x = np.where(np.isnan(func_result), np.nan, df['max']) except KeyError: func_result = np.full_like(upper, np.nan) x = func_result.copy() From 205a311b1c3019d7d5a73013cdb9539bac1fec8e Mon Sep 17 00:00:00 2001 From: Cliff Hansen Date: Wed, 7 Dec 2022 15:26:20 -0700 Subject: [PATCH 2/6] whatsnew --- docs/sphinx/source/whatsnew/v0.9.4.rst | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/docs/sphinx/source/whatsnew/v0.9.4.rst b/docs/sphinx/source/whatsnew/v0.9.4.rst index c042f6aaa2..abbabc2840 100644 --- a/docs/sphinx/source/whatsnew/v0.9.4.rst +++ b/docs/sphinx/source/whatsnew/v0.9.4.rst @@ -12,19 +12,21 @@ Enhancements * Multiple code style issues fixed that were reported by LGTM analysis. (:issue:`1275`, :pull:`1559`) * Added a direct IAM model :py:func:`pvlib.iam.schlick` which can be used with :py:func:`~pvlib.iam.marion_diffuse`, and a diffuse IAM model - :py:func:`pvlib.iam.schlick_diffuse` (:pull:`1562`, :issue:`1564`) + :py:func:`pvlib.iam.schlick_diffuse`. (:pull:`1562`, :issue:`1564`) * Added a function to calculate one of GHI, DHI, and DNI from values of the other two. - :py:func:`~pvlib.irradiance.complete_irradiance` + :py:func:`~pvlib.irradiance.complete_irradiance`. (:issue:`1565`, :pull:`1567`) * Add optional ``return_components`` parameter to :py:func:`pvlib.irradiance.haydavies` to return - individual diffuse irradiance components (:issue:`1553`, :pull:`1568`) + individual diffuse irradiance components. (:issue:`1553`, :pull:`1568`) Bug fixes ~~~~~~~~~ * Fixed bug in :py:func:`pvlib.shading.masking_angle` and :py:func:`pvlib.bifacial.infinite_sheds._ground_angle` - where zero ``gcr`` input caused a ZeroDivisionError (:issue:`1576`, :pull:`1589`) + where zero ``gcr`` input caused a ZeroDivisionError. (:issue:`1576`, :pull:`1589`) +* Fixed bug in :py:func:`pvlib.tools._golden_sect_DataFrame` so that a result is returned when the search + interval is length 0 (which occurs in :py:func:`pvlib.pvsystem.singlediode` if v_oc is 0.) (:issue:`1603`, :pull:`1606`) Testing ~~~~~~~ From 45794788d56ee212cfe09d1df8de8dbabc78e6b5 Mon Sep 17 00:00:00 2001 From: Cliff Hansen Date: Wed, 7 Dec 2022 15:28:33 -0700 Subject: [PATCH 3/6] clarify error message --- pvlib/tools.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pvlib/tools.py b/pvlib/tools.py index 8550904fbb..e7223be105 100644 --- a/pvlib/tools.py +++ b/pvlib/tools.py @@ -342,7 +342,7 @@ def _golden_sect_DataFrame(params, lower, upper, func, atol=1e-8): pvlib.singlediode._pwr_optfcn """ if np.any(upper - lower < 0.): - raise ValueError('upper > lower is required') + raise ValueError('upper >= lower is required') phim1 = (np.sqrt(5) - 1) / 2 From 2ca002fba88c7d4d015f40d1624fd1e5c67b09f7 Mon Sep 17 00:00:00 2001 From: Cliff Hansen Date: Thu, 8 Dec 2022 09:17:23 -0700 Subject: [PATCH 4/6] improve from review --- pvlib/tools.py | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/pvlib/tools.py b/pvlib/tools.py index e7223be105..39ea77f442 100644 --- a/pvlib/tools.py +++ b/pvlib/tools.py @@ -350,13 +350,7 @@ def _golden_sect_DataFrame(params, lower, upper, func, atol=1e-8): df['VH'] = upper df['VL'] = lower - - # handle all NaN case gracefully - with warnings.catch_warnings(): - warnings.filterwarnings(action='ignore', - message='All-NaN slice encountered') - # handle upper == lower here - converged = np.nanmax(np.abs(upper - lower)) < atol + converged = False while not converged: @@ -373,10 +367,11 @@ def _golden_sect_DataFrame(params, lower, upper, func, atol=1e-8): err = abs(df['V2'] - df['V1']) - converged = np.all(err[~np.isnan(err)] < atol) - - # works with single value because err is np.float64 -# converged = (err[~np.isnan(err)] < atol).all() + # handle all NaN case gracefully + with warnings.catch_warnings(): + warnings.filterwarnings(action='ignore', + message='All-NaN slice encountered') + converged = np.all(err[~np.isnan(err)] < atol) # best estimate of location of maximum df['max'] = 0.5 * (df['V1'] + df['V2']) From 1d3df379677ffb01a4524099f6b7257adb4002bd Mon Sep 17 00:00:00 2001 From: Cliff Hansen Date: Thu, 8 Dec 2022 09:19:05 -0700 Subject: [PATCH 5/6] length 1 array test --- pvlib/tests/test_tools.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/pvlib/tests/test_tools.py b/pvlib/tests/test_tools.py index 075c29968f..4d5312088b 100644 --- a/pvlib/tests/test_tools.py +++ b/pvlib/tests/test_tools.py @@ -45,7 +45,7 @@ def test__golden_sect_DataFrame_vector(): v, x = tools._golden_sect_DataFrame(params, lower, upper, _obj_test_golden_sect) assert np.allclose(x, expected, atol=1e-8) - # upper and lower bounds equal + # some upper and lower bounds equal params = {'c': np.array([1., 2., 1.]), 'n': np.array([1., 1., 1.])} lower = np.array([0., 0.001, 1.]) upper = np.array([1., 1.2, 1.]) @@ -53,6 +53,14 @@ def test__golden_sect_DataFrame_vector(): v, x = tools._golden_sect_DataFrame(params, lower, upper, _obj_test_golden_sect) assert np.allclose(x, expected, atol=1e-8) + # all upper and lower bounds equal, arrays of length 1 + params = {'c': np.array([1.]), 'n': np.array([1.])} + lower = np.array([1.]) + upper = np.array([1.]) + expected = np.array([1.]) # x values for maxima + v, x = tools._golden_sect_DataFrame(params, lower, upper, + _obj_test_golden_sect) + assert np.allclose(x, expected, atol=1e-8) def test__golden_sect_DataFrame_nans(): From 42f97867262a14677319085fcedde0fdf82ab0d2 Mon Sep 17 00:00:00 2001 From: Cliff Hansen Date: Fri, 9 Dec 2022 10:30:12 -0700 Subject: [PATCH 6/6] remove try/except not needed --- pvlib/tools.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/pvlib/tools.py b/pvlib/tools.py index 39ea77f442..229c5dd444 100644 --- a/pvlib/tools.py +++ b/pvlib/tools.py @@ -375,12 +375,8 @@ def _golden_sect_DataFrame(params, lower, upper, func, atol=1e-8): # best estimate of location of maximum df['max'] = 0.5 * (df['V1'] + df['V2']) - try: - func_result = func(df, 'max') - x = np.where(np.isnan(func_result), np.nan, df['max']) - except KeyError: - func_result = np.full_like(upper, np.nan) - x = func_result.copy() + func_result = func(df, 'max') + x = np.where(np.isnan(func_result), np.nan, df['max']) return func_result, x