Gold I-V dataset with benchmarking #426

markcampanelli · 2018-02-13T10:20:23Z

pvlib python pull request guidelines

Thank you for your contribution to pvlib python!

You may submit a pull request with your code at any stage of completion, however, before the code can be merged the following items must be addressed:

Partly addresses issue Test singlediode() implementations using a "gold" function and data set #411
Fully tested. Added and/or modified tests to ensure correct behavior for all reasonable inputs. Tests must pass on the TravisCI and Appveyor testing services.
Code quality and style is sufficient. Passes git diff upstream/master -u -- "*.py" | flake8 --diff and/or landscape.io linting service.
New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
Updates entries to docs/sphinx/source/api.rst for API changes.
Adds description and name entries in the appropriate docs/sphinx/source/whatsnew file for all changes.

Please don't hesitate to ask for help if you're unsure of how to accomplish any of the above. You may delete all of these instructions except for the list above.

Brief description of the problem and proposed solution (if not already fully described in the issue linked to above):

…_ivcurve

markcampanelli · 2018-02-13T10:29:10Z

Here is some code for others to reproduce computations. To run this do the following:

$ pip install pyinterval # first time only
$ python gold_dataset.py # Will rewrite test.json file with gold dataset

pvlib/data/gold/worst_pvlib_computaitons.json contains the worst computations from my laptop on python 1.0.0 with numpy 1.14.0 and pvlib 0.5.1+34.g329450a.dirty

cwhanse · 2018-06-21T15:22:48Z

pvlib/data/gold/sdm.py

+        current
+
+
+def bootstrap_si_device_bishop(cell_area_cm2=4., N_s=1, ideality_factor=1.5):


I'd prefer create_si_device_bishop

cwhanse · 2018-06-21T15:24:34Z

pvlib/data/gold/sdm.py

+
+def bootstrap_si_device_bishop(cell_area_cm2=4., N_s=1, ideality_factor=1.5):
+    """
+    Create c-Si device based on cell parameters in Bishop's 1988 paper Fig. 6.


Create a De Soto model for a c-Si device...

cwhanse · 2018-06-21T15:27:37Z

pvlib/data/gold/sdm.py

+
+
+def bootstrap_si_cell_large():
+    """


Prefer create_si_cell_large

cwhanse · 2018-06-21T15:27:58Z

pvlib/data/gold/sdm.py

+def bootstrap_si_cell_large():
+    """
+    Create a single-large-area-cell c-Si device based on a SunPower module.
+    """


Add note that we are creating a De Soto model for the cell

cwhanse · 2018-06-21T15:35:08Z

pvlib/data/gold/sdm.py

+    device.V_mp_ref = device.V_mp_ref / Ns_module
+    device.a_ref = device.a_ref / Ns_module
+    device.R_s = device.R_s / Ns_module
+    device.R_sh_ref = Ns_module * device.R_sh_ref


Should this be Ns_module / device.R_sh_ref ?

No, you need to multiply that one.

cwhanse · 2018-06-21T15:36:05Z

pvlib/data/gold/sdm.py

+def make_gold_dataset():
+    """
+    Make a gold dataset of I-V curves that accurately solve single-diode model.
+


that accurately solve the single diode equation.

...that are accurate solutions to...

cwhanse · 2018-06-21T15:40:23Z

pvlib/data/gold/sdm.py

+    Returns
+    -------
+    gold_dataset : dict
+        The gold data are returned in a dict with metadata (e.g., timestamp,


Is it practical to outline the gold_dataset dict here (i.e., list and describe each key), the device dict, and a list member dict for a device?

cwhanse · 2018-06-21T15:43:40Z

pvlib/data/gold/sdm.py

+    returned in a python dict that is in a lossless, machine-interoperable
+    format, which is the expected input to most of the other functions in this
+    python module. (Other functions can convert this dict to a human-readable
+    format.) An explict method of Bishop [1] is used to compute (V, I) pairs


Would help to name the 'other functions'

cwhanse · 2018-06-21T15:44:18Z

pvlib/data/gold/sdm.py

+    return gold_dataset
+
+
+def convert_gold_dataset(gold_dataset):


maybe add _to_pvlib to function name

On second thought, gold_dataset_to_pvlib

cwhanse · 2018-06-21T15:45:39Z

pvlib/data/gold/sdm.py

+    return gold_dataset_converted
+
+
+def pretty_print_gold_dataset(gold_dataset):


gold_dataset_to_csv

cwhanse · 2018-06-21T15:50:07Z

pvlib/data/gold/sdm.py

+            # Load G, T, and 5 single-diode model parameters for each I-V curve
+            # Floats converted from lossless, machine-interoperable hex format
+
+            iv_curve["G"] = float.fromhex(iv_curve["G"])


should we use the pvlib keys? effective_irradiance instead of G, temp_cell instead of T, etc.

cwhanse · 2018-06-21T15:52:37Z

pvlib/data/gold/sdm.py

+        See ``make_gold_dataset()``.
+
+    test_func : dict
+        A dictionary of functions to guage. Supported keys to functions are


adriesse · 2018-06-21T16:07:29Z

Sorry, I'm not too clear on the question here. There's been too much going on!

@thunderfish24 I understand your perspective. The primary reason I requested some refactoring was to make the code easier to understand and review for correctness. I think the code is in good shape otherwise.

I am unlikely to use this feature, so I ask @cwhanse @adriesse @mikofski and any others for their opinion of how to proceed.

cwhanse · 2018-06-21T16:17:30Z

@adriesse Will is inviting you to review the pull request. OK if you don't have time, since both Will and I have done so. The PR consists of two basic components: a function to precisely calculate sets of IV curves (a number of example devices are included), and a function to compare between two IV curve calculators (think of a function that runs and compares two functions). I think Will's major interest is, do you think this capability is appropriate for and useful to pvlib?

adriesse

Overall impressions:

There are some really long functions here, and it would be nice to chop them up. I think one way to do this might be to not treat the whole gold set as a single entity but process (and store/read) them one device or even one curve at a time.

The storage format was discussed a while back. Given the size of your tolerance I think the lossy csv format is more than adequate. You can also flatten the data into a 2d table and read/write that with pandas and even put a few header lines with extra info pretty easily. I think multiple files would be appropriate--there won't be hundreds.

Anyway, even if neither of those two ideas is appealing, there are some other ideas for improvements already. It is a big contribution overall, and I think it would be fine to take some extra time to hone it further.

adriesse · 2018-06-21T20:39:18Z

pvlib/data/gold/sdm.py

+    device.PTC = None
+
+    return device
+


This leaves me searching whether you kept attribute of the device you copied. Wouldn't it be cleaner to just create a new device from scratch?

adriesse · 2018-06-21T20:40:53Z

pvlib/data/gold/sdm.py

+    device.V_mp_ref = device.V_mp_ref / Ns_module
+    device.a_ref = device.a_ref / Ns_module
+    device.R_s = device.R_s / Ns_module
+    device.R_sh_ref = Ns_module * device.R_sh_ref


No, you need to multiply that one.

adriesse · 2018-06-21T20:42:00Z

pvlib/data/gold/sdm.py

+def make_gold_dataset():
+    """
+    Make a gold dataset of I-V curves that accurately solve single-diode model.
+


...that are accurate solutions to...

adriesse · 2018-06-21T21:03:58Z

pvlib/data/gold/sdm.py

+
+    From a gold dataset in the lossless, machine-interoperable format, create
+    and return the dataset as a human-readale, comma-separated-value (csv)
+    string.


adriesse · 2018-06-21T21:58:28Z

pvlib/data/gold/sdm.py

+                            # Don't include entire curve in exception message
+                            del device_dict["iv_curves"][-1]["v_gold"]
+                            del device_dict["iv_curves"][-1]["i_gold"]
+                            raise ValueError("Residual interval {} not in {}, \


Does this ever happen??

adriesse · 2018-06-21T22:00:53Z

pvlib/data/gold/sdm.py

+                            interval(nNsVth)) - interval(g_sh) *
+                            (interval(v) + interval(i)*interval(r_s)) -
+                            interval(i) for v, i in zip(v_gold, i_gold)]
+


What's the advantage of using intervals here, where all the intervals are single numbers?

adriesse · 2018-06-21T22:02:21Z

pvlib/data/gold/sdm.py

+                    for v, i, current_sum_residual_interval in zip(
+                            v_gold, i_gold,
+                            current_sum_residual_interval_list):
+                        if current_sum_residual_interval not in tol_interval:


What is the advantage of this over np.isclose()?

adriesse · 2018-06-21T22:07:22Z

pvlib/data/gold/sdm.py

+            device_dict["i_from_v"]["performance"]["time_s"]["std"] = \
+                numpy.std(device_dict["i_from_v"]
+                          ["performance"]["time_s"]["data"], ddof=1)
+


This block is quite hard for me to parse.

adriesse · 2018-06-22T11:14:28Z

pvlib/data/gold/sdm.py

+        pvlib.pvsystem.retrieve_sam('cecmod').SunPower_SPR_E20_327,
+        pvlib.pvsystem.retrieve_sam('cecmod').First_Solar_FS_495
+    ]
+


What do you think about copying the parameters for all 5 devices into golden_devices.csv or something similar? Then you just need a single read.

markcampanelli · 2019-05-15T18:22:10Z

ci/requirements-py34.yml

@@ -17,5 +17,7 @@ dependencies:
    - nose
    - pip:
        - coveralls
+        - pvfactors==1.0.1


@anomam Do you know if I'm fixing or breaking something here? (Is pvfactors 1.0.1 really not supported in Python 3.4?)

That's a good point @markcampanelli , starting with version 1.0.0 pvfactors should work with earlier versions of pandas, and therefore be compatible with Python 3.4.
Could you post an issue in pvfactors by any chance? A PR would be very much welcome as well :)

Python 3.4 hit "end of life" a few months ago. I don't plan to support it going forward.

SunPower/pvfactors#49

markcampanelli · 2019-05-16T17:00:47Z

It looks like scipy/scipy#8028 may have fixed the significant issue with a scipy.special.wlambert calculation that I saw earlier on my machine (see way above). With scipy 1.2.1, scipy.special.lambertw(1.22045782051e+304) now properly computes 693.6431367052406+0j. I am now investigating the new computations and making sure that remaining ones with large residuals can be reasonably explained as being from numerics and not bugs. After this I will try to improve improve code naming and organization based on earlier suggestions before requesting another review.

…gold_dataset

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

Closing this in favor of #1573.

Mark Campanelli and others added 28 commits June 14, 2017 22:28

Add draft i_from_v_alt function with tests

9ab2e95

Merge branch 'master' of github.com:pvlib/pvlib-python

a66abc4

Better comments and more explicit typing

a19447b

Use transform from shunt resistance to shunt conductance

76616d5

Add v_from_i_alt() with initial tests and use np.where

e35412f

Use test fixtures

82217fd

Add @requires_scipy to test fixtures

135ce4c

More current_sum_at_diode_node() tests and using fixture

f927aef

Naming, documentation, and formatting

741a7f4

Deprecate replaced functions and flake8

c9129dc

Merge branch 'master' of github.com:pvlib/pvlib-python

3aa150d

Add release documentation and flake8 again

85e957d

Replace deprecated function usages and update test_singlediode_series…

fb22171

…_ivcurve

Merge branch 'master' of github.com:pvlib/pvlib-python

275eccc

Conform to existing API

cdd2bd9

Run flake8

223275a

Implement some code quality suggestions

6b50ad9

Remove extraneous print statements

e4e09be

Better docstrings

37484f1

Fix parameter ranges in docstrings

f9c4c82

Add test that overflows lambertw arg

26eabde

Add test for mixed solution types logic

4d7754d

Use broadcast_arrays for cleaner code

03ae51e

One more simplification

6c410d2

Better use of broadcast_arrays

19af023

Merge branch 'master' of github.com:pvlib/pvlib-python

0cbf4bd

Merge branch 'master' of github.com:pvlib/pvlib-python into gold_dataset

329450a

Initial code with example output

c96795d

markcampanelli mentioned this pull request Feb 13, 2018

Test singlediode() implementations using a "gold" function and data set #411

Closed

cwhanse reviewed Jun 21, 2018

View reviewed changes

pvlib/data/gold/sdm.py

def bootstrap_si_cell_large():

"""

Copy link

Member

cwhanse Jun 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer create_si_cell_large

cwhanse reviewed Jun 21, 2018

View reviewed changes

pvlib/data/gold/sdm.py

return gold_dataset_converted

def pretty_print_gold_dataset(gold_dataset):

Copy link

Member

cwhanse Jun 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gold_dataset_to_csv

cwhanse reviewed Jun 21, 2018

View reviewed changes

adriesse reviewed Jun 21, 2018

View reviewed changes

adriesse reviewed Jun 22, 2018

View reviewed changes

wholmgren mentioned this pull request Jun 29, 2018

ENH: add methods and tests for a explicit IV curve calculation of single-diode model #409

Merged

6 tasks

markcampanelli added 2 commits May 15, 2019 12:15

Merge upstream master

5eca66f

Make requirements consistent for py34

aee313e

markcampanelli commented May 15, 2019

View reviewed changes

markcampanelli added 3 commits May 15, 2019 12:24

Add GH issue

e6e6dc7

Merge branch 'master' of github.com:pvlib/pvlib-python into gold_dataset

6ad79ec

Merge upstream

f9dc4e7

markcampanelli added 3 commits May 17, 2019 15:19

Update outputs and fix typos and sticklers

13b6042

Remove pvfactors for Python 3.4

0041d4e

Merge branch 'master' of github.com:markcampanelli/pvlib-python into …

d85aac6

…gold_dataset

markcampanelli closed this Dec 4, 2022

markcampanelli mentioned this pull request Dec 4, 2022

Test pvsystem.singlediode against precisely generated IV curves #1573

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gold I-V dataset with benchmarking #426

Gold I-V dataset with benchmarking #426

markcampanelli commented Feb 13, 2018 •

edited

Loading

markcampanelli commented Feb 13, 2018

cwhanse Jun 21, 2018

cwhanse Jun 21, 2018

cwhanse Jun 21, 2018

cwhanse Jun 21, 2018

cwhanse Jun 21, 2018

adriesse Jun 21, 2018

cwhanse Jun 21, 2018

adriesse Jun 21, 2018

cwhanse Jun 21, 2018

cwhanse Jun 21, 2018

cwhanse Jun 21, 2018

cwhanse Jun 21, 2018

cwhanse Jun 21, 2018

cwhanse Jun 21, 2018

cwhanse Jun 21, 2018

adriesse commented Jun 21, 2018

cwhanse commented Jun 21, 2018 •

edited

Loading

adriesse left a comment

adriesse Jun 21, 2018

adriesse Jun 21, 2018

adriesse Jun 21, 2018

adriesse Jun 21, 2018

adriesse Jun 21, 2018

adriesse Jun 21, 2018

adriesse Jun 21, 2018

adriesse Jun 21, 2018

adriesse Jun 22, 2018

markcampanelli May 15, 2019

anomam May 16, 2019

wholmgren May 16, 2019

markcampanelli May 17, 2019

markcampanelli commented May 16, 2019 •

edited

Loading

markcampanelli commented Dec 4, 2022

		current


		def bootstrap_si_device_bishop(cell_area_cm2=4., N_s=1, ideality_factor=1.5):

		return gold_dataset_converted


		def pretty_print_gold_dataset(gold_dataset):

Gold I-V dataset with benchmarking #426

Gold I-V dataset with benchmarking #426

Conversation

markcampanelli commented Feb 13, 2018 • edited Loading

pvlib python pull request guidelines

markcampanelli commented Feb 13, 2018

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

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

adriesse commented Jun 21, 2018

cwhanse commented Jun 21, 2018 • edited Loading

adriesse 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markcampanelli commented May 16, 2019 • edited Loading

markcampanelli commented Dec 4, 2022

markcampanelli commented Feb 13, 2018 •

edited

Loading

cwhanse commented Jun 21, 2018 •

edited

Loading

markcampanelli commented May 16, 2019 •

edited

Loading