Test pvsystem.singlediode against precisely generated IV curves #1573

reepoi · 2022-10-16T22:25:35Z

Closes Test singlediode() implementations using a "gold" function and data set #411
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.

Generated IV curves are added to pvlib/data to test the precision of pvsystem.singlediode, pvsystem.v_from_i, and pvsystem.i_from_v. The parameters of the single diode equation that generated each IV curve are in CSV files, and the IV curve data are in JSON files following this JSON schema. i_x and i_xx are added in addition to the fields of the JSON schema since they are also calculated by pvsystem.singlediode.
The generated IV curve data are calculated using the functions in the ivcurves repository. These functions use the arbitrary precision math library mpmath. The data may be reproduced by checking out the code of this commit of ivcurves and running the command

python3 ivcurves/precise.py --save-json test_sets

The data are in the files test_sets/case1.json and test_sets/case2.json.

…nd newton

pvlib/pvsystem.py

…cise-ivcurves-tests

reepoi · 2022-11-17T21:35:40Z

I updated the description of this pull request. The test cases added now test:

pvsystem.singlediode with ivcurve_pnts=None and all options for method: checks all the returned values.
pvsystem.singlediode with ivcurve_pnts=100 and method='lambertw': checks only the returned current and voltage values.
pvsystem.v_from_i and pvsystem.i_from_v with all options for method: checks the returned voltage or current values.

The tests for pvsystem.v_from_i and pvsystem.i_from_v indirectly test the precision of pvsystem.singlediode with methods brentq and newton when ivcurves_pnts=100 since it they all eventually call pvsystem.bishop88.

cwhanse · 2022-11-30T17:43:08Z

Ready for review. @markcampanelli would appreciate your input if this closes #411

…cise-ivcurves-tests

markcampanelli

Thanks for this submission. Generally looks good.

The big thing I would like to see is some better documentation in this repo (mostly for posterity) about how the precise I-V data points were generated elsewhere. For example, how did you choose the precision cutoff using mpmath with respect to the precision used in the pvsystem.singlediode calculations? The choice of constants in the upstream calculation could also have a better "paper trail".

markcampanelli · 2022-12-04T21:03:11Z

pvlib/data/precise_iv_curves_parameter_sets1.csv

@@ -0,0 +1,33 @@
+Index,photocurrent,saturation_current,resistance_series,resistance_shunt,n,cells_in_series
+1,1.0,5e-10,0.1,300,1.01,72


Because this is a Python repo, you might consider indexing from 0, not 1.

markcampanelli · 2022-12-04T21:06:08Z

pvlib/data/precise_iv_curves1.json

+  "cells_in_series": 72,
+  "IV Curves": [
+    {
+      "Index": 1,


Because this is a Python repo, you might consider indexing from 0, not 1.

markcampanelli · 2022-12-04T21:18:24Z

pvlib/tests/test_singlediode.py

+    joined['Boltzman'] = 1.380649e-23
+    joined['Electron Charge'] = 1.60217663e-19


For better traceability, I would think that the choice of values for these constants should documented as part of the files containing the parameters used to precisely compute the curves.

For comparison, scipy==1.8.1 gives:

scipy.constants.value("Boltzmann constant") = 1.380649e-23 J/K
scipy.constants.value("elementary charge") = 1.602176634e-19 C

Also see #483

Ok, I updated these constants to match The SI Brochure (https://www.bipm.org/en/publications/si-brochure) and cited that document in the docstring. In that docstring, I also added the precision cutoff of mpmath and more detail about the precision of the IV curve data. I avoided mentioning cwhanse/ivcurves, but maybe we should reference it. There is much more documentation and code that would help someone understand how the IV curve data was generated.

Also, if we use scipy.constants, pvlib may need to update its minimum SciPy version requirement because Boltzmann's constant changes depending on the SciPy version. The value for SciPy 1.9.3 is 1.602176634e-19 C, which matches the value hard-coded here; however, the value is 1.6021766208e-19 C for SciPy 1.2.0. It changes in SciPy 1.4.0, and pvlib's minimum version of SciPy is 1.2.0.

@pvlib/pvlib-maintainer I think we should advance scipy version and use the 2019 values, which are defined to be exact rather than with uncertainty.

markcampanelli · 2022-12-04T21:32:06Z

pvlib/tests/test_singlediode.py

+    assert np.allclose(pc['i_sc'], outs['i_sc'], atol=1e-10, rtol=0)
+    assert np.allclose(pc['v_oc'], outs['v_oc'], atol=1e-10, rtol=0)
+    assert np.allclose(pc['i_mp'], outs['i_mp'], atol=5e-8, rtol=0)
+    assert np.allclose(pc['v_mp'], outs['v_mp'], atol=8e-7, rtol=0)
+    assert np.allclose(pc['p_mp'], outs['p_mp'], atol=1e-10, rtol=0)
+    assert np.allclose(pc['i_x'], outs['i_x'], atol=1e-10, rtol=0)
+    assert np.allclose(pc['i_xx'], outs['i_xx'], atol=8e-8, rtol=0)


I'm super curious how these tolerances might be tightened for 'lambertw', 'brentq', vs. 'newton'.

And I'm curious why there are test failures only on ubuntu and python <=3.9 for the 'lambertw' case. I suspect something buried fairly deep behind scipy.special.lambertw which we won't solve.

For this failed test, the 30th i_xx value differs by 1e-6. All of the tests related to this pull request pass with atol=1e-6 for the i_xx value.

@reepoi I think there's a mistake in the precise curve library somewhere.

markcampanelli · 2022-12-04T22:16:53Z

Ready for review. @markcampanelli would appreciate your input if this closes #411

I think this addresses #411, so that can be closed once this merges.

I have closed #426, which this substitutes.

…cise-ivcurves-tests

kandersolar · 2023-01-17T15:14:44Z

The current test failures are only on linux, only occur for numpy>=1.22.0, and have to do with precision checks, so I suspect this is due to the increased SIMD functionality added in numpy 1.22.0.

In the past we have relaxed test tolerances to accommodate the slight numerical variation, but in this specific case that seems rather self-defeating :) It is possible to disable the relevant SIMD features by setting a magic environment variable when executing the tests (#1448 (comment)), but that would be a global change that affects all tests, which I'm not sure is desirable either. I'm not sure what to suggest here.

cwhanse · 2023-01-23T16:57:17Z

The current test failures are only on linux, only occur for numpy>=1.22.0, and have to do with precision checks, so I suspect this is due to the increased SIMD functionality added in numpy 1.22.0.

In the past we have relaxed test tolerances to accommodate the slight numerical variation, but in this specific case that seems rather self-defeating :)

My perspective is that the test tolerance is a measure of the pvlib functions' accuracy. Six digits (worst case) is at least useful for energy simulations and may also help provide context for other use cases. At least it establishes a benchmark from which we can measure improvements.

kandersolar · 2023-03-07T17:16:08Z

@cwhanse a lot has happened in the https://github.com/cwhanse/ivcurves repository in the last couple months. Are any corresponding updates required in this PR?

cwhanse · 2023-03-07T17:30:03Z

We may need to replace the json files

…cise-ivcurves-tests

reepoi · 2023-03-08T03:17:59Z

I regenerated the JSON files using the latest version of the https://github.com/cwhanse/ivcurves repository and there is no difference between them and the JSON files in this PR.

kandersolar

A few minor docs suggestions but otherwise LGTM. Thanks @reepoi!

docs/sphinx/source/whatsnew/v0.9.5.rst

pvlib/tests/test_singlediode.py

kandersolar · 2023-03-15T16:35:46Z

Thanks @reepoi for the contribution and @markcampanelli and @cwhanse for the reviews!

reepoi added 8 commits September 30, 2022 12:01

start to adding tests from data in ivcurves repo

b92d376

correcting Vth calculation

bebcfef

making current increasing for numpy linear interp

74c5f8c

singlediode ivcurve_pnts does not give negative voltage with brentq a…

85f83af

…nd newton

adding test for singlediode log spacing methods

f165e08

adding assertions for i_x and i_xx

f27b97d

adding i_x, i_xx iv curve data

64d6cef

updating whatsnew

8019f2c

kandersolar mentioned this pull request Oct 17, 2022

Schema feedback cwhanse/ivcurves#12

Closed

cwhanse added enhancement testing labels Oct 26, 2022

cwhanse reviewed Oct 28, 2022

View reviewed changes

pvlib/pvsystem.py Outdated Show resolved Hide resolved

reepoi added 6 commits October 30, 2022 19:28

reverting changes to pvsystem.singlediode

2484dca

move precise iv curve fixtures into test_singlediode

dcaddc8

updating precise iv curve json to match new schema

3115bba

Merge branch 'main' of https://github.com/pvlib/pvlib-python into pre…

150ee02

…cise-ivcurves-tests

testing v_from_i, i_from_v

1c0f2b6

small refactor

a98825e

cwhanse added this to the 0.9.4 milestone Nov 30, 2022

reepoi added 2 commits December 4, 2022 13:18

Merge branch 'main' of https://github.com/pvlib/pvlib-python into pre…

64f9c02

…cise-ivcurves-tests

small refactor and update whatsnew

3537878

markcampanelli suggested changes Dec 4, 2022

View reviewed changes

markcampanelli mentioned this pull request Dec 4, 2022

Gold I-V dataset with benchmarking #426

Closed

6 tasks

kandersolar mentioned this pull request Dec 9, 2022

v0.9.4 release plan #1608

Closed

9 tasks

kandersolar modified the milestones: 0.9.4, 0.9.5 Dec 14, 2022

changing Boltzmann's constant

3670964

reepoi added 2 commits December 18, 2022 12:11

add more to precise iv curve docstring

a27db19

Merge branch 'main' of https://github.com/pvlib/pvlib-python into pre…

2c2ca4a

…cise-ivcurves-tests

cwhanse mentioned this pull request Dec 19, 2022

Use scipy.constants #1617

Merged

6 tasks

reepoi added 6 commits January 9, 2023 18:47

Merge branch 'main' of https://github.com/pvlib/pvlib-python into pre…

5178f87

…cise-ivcurves-tests

use SciPy constants

a1040d3

restore whatsnew 0.9.4

99b9830

relax i_xx error tolerance

2471893

trying lower i_xx atol

a13583b

update generated iv curves after finding bug in their calcuation

020e363

increasing i_xx atol, and adding comment about atol on mac, windows

be34c09

kandersolar mentioned this pull request Mar 2, 2023

v0.9.5 release planning #1681

Closed

12 tasks

reepoi added 2 commits March 7, 2023 18:55

Merge branch 'main' of https://github.com/pvlib/pvlib-python into pre…

e966f20

…cise-ivcurves-tests

update whatsnew

4b3016d

kandersolar approved these changes Mar 8, 2023

View reviewed changes

docs/sphinx/source/whatsnew/v0.9.5.rst Outdated Show resolved Hide resolved

docs/sphinx/source/whatsnew/v0.9.5.rst Show resolved Hide resolved

pvlib/tests/test_singlediode.py Outdated Show resolved Hide resolved

update whatsnew and add references to test docs

5a6b631

cwhanse approved these changes Mar 15, 2023

View reviewed changes

kandersolar merged commit 13744ca into pvlib:main Mar 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test pvsystem.singlediode against precisely generated IV curves #1573

Test pvsystem.singlediode against precisely generated IV curves #1573

reepoi commented Oct 16, 2022 •

edited by cwhanse

Loading

reepoi commented Nov 17, 2022 •

edited by cwhanse

Loading

cwhanse commented Nov 30, 2022

markcampanelli left a comment

markcampanelli Dec 4, 2022

markcampanelli Dec 4, 2022

markcampanelli Dec 4, 2022

markcampanelli Dec 4, 2022

reepoi Dec 18, 2022

cwhanse Dec 19, 2022

markcampanelli Dec 4, 2022

cwhanse Jan 10, 2023

reepoi Jan 11, 2023

cwhanse Jan 11, 2023

markcampanelli commented Dec 4, 2022

kandersolar commented Jan 17, 2023

cwhanse commented Jan 23, 2023

kandersolar commented Mar 7, 2023

cwhanse commented Mar 7, 2023

reepoi commented Mar 8, 2023

kandersolar left a comment

kandersolar commented Mar 15, 2023

		@@ -0,0 +1,33 @@
		Index,photocurrent,saturation_current,resistance_series,resistance_shunt,n,cells_in_series
		1,1.0,5e-10,0.1,300,1.01,72

		joined['Boltzman'] = 1.380649e-23
		joined['Electron Charge'] = 1.60217663e-19

Test pvsystem.singlediode against precisely generated IV curves #1573

Test pvsystem.singlediode against precisely generated IV curves #1573

Conversation

reepoi commented Oct 16, 2022 • edited by cwhanse Loading

reepoi commented Nov 17, 2022 • edited by cwhanse Loading

cwhanse commented Nov 30, 2022

markcampanelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markcampanelli commented Dec 4, 2022

kandersolar commented Jan 17, 2023

cwhanse commented Jan 23, 2023

kandersolar commented Mar 7, 2023

cwhanse commented Mar 7, 2023

reepoi commented Mar 8, 2023

kandersolar left a comment

Choose a reason for hiding this comment

kandersolar commented Mar 15, 2023

reepoi commented Oct 16, 2022 •

edited by cwhanse

Loading

reepoi commented Nov 17, 2022 •

edited by cwhanse

Loading