Update SAM libraries #444

cwhanse · 2018-03-30T21:50:02Z

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:

Closes update sam library files #440
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):

wholmgren · 2018-03-30T21:58:48Z

YES!!!

This mostly looks good to me. It looks like you accidentally changed the line for looking up the adr inverter file instead of the cec inverter file.

We should add a comment to the whats new file.

I think we should remove the old files, too, but that is up for debate.

git rm pvlib/data/sam-library-cec-inverters-2015-6-30.csv
git rm pvlib/data/sam-library-cec-modules-2015-6-30.csv

cwhanse · 2018-03-30T22:00:32Z

We can remove the old files. I checked that the new files are adding entries, there aren't any old ones that will disappear. I checked that the pvlib function reads the new files. And, it figures that once the commit got through it would have a mistake in it.

…

________________________________ From: Will Holmgren <[email protected]> Sent: Friday, March 30, 2018 3:58 PM To: pvlib/pvlib-python Cc: Hansen, Clifford W; Author Subject: [EXTERNAL] Re: [pvlib/pvlib-python] Update SAM libraries (#444) YES!!! This mostly looks good to me. It looks like you accidentally changed the line for looking up the adr inverter file instead of the cec inverter file. We should add a comment to the whats new file. I think we should remove the old files, too, but that is up for debate. git rm pvlib/data/sam-library-cec-inverters-2015-6-30.csv git rm pvlib/data/sam-library-cec-modules-2015-6-30.csv - You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#444 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AFJNLyzeSF5HudJiEgGkDXotkTX0mqQ1ks5tjqqZgaJpZM4TCLDo>.

cwhanse · 2018-03-30T22:05:47Z

Fixed the inverter file lines

wholmgren · 2018-03-30T22:36:15Z

It appears that the "Example Module" parameters are slightly different, so that explains a few of the failing tests.

I think the others are failing because some of the fields are now read in as strings instead of as numbers. Maybe there is a slight change to the format or a new name that prevents pandas from assuming that the fields are numeric?

wholmgren · 2018-03-30T23:05:39Z

I think the problem is line 2508 in the inverters file:

LeadSolar Energy: LS600T [208V] 208V [CEC 2018],208,500,75790.4,36,-nan(ind),1.22899e-07,-0.0251886,-nan(ind),0.0798754,1,45,2105.29,27,45

cwhanse · 2018-03-30T23:14:45Z

Glad you tracked that line down. I got as far are 'there's something different in format between these files'

cwhanse · 2018-03-30T23:23:01Z

Yep, that single inverter entry is missing values for two parameters. I've opened an issue with the SAM team. Want to delete the line (there's a 2014 entry for the same inverter, the entry with missing values is 2018), or hold off and see what the SAM folks do?

remove bad inverter line, update test

mikofski · 2018-03-31T09:23:24Z

FYI: I've add the CEC modules to pvfree, and updated the CEC inverters to the latest 3/18/2018 SAM dump. Try it:

either https://pvfree.alwaysdata.net/api/v1/cecmodule/?format=json&Name__icontains=spr-e20
or https://pvfree.herokuapp.com/api/v1/cecmodule/?format=json&Name__icontains=spr-e20

yields ...

{"meta": {"limit": 20, "next": null, "offset": 0, "previous": null, "total_count": 10}, "objects": [
  {"A_c": 1.244, "Adjust": -15.41, "BIPV": false, "Date": "2013-01-14", "I_L_ref": 6.436, "I_mp_ref": 6.05, "I_o_ref": 2.43e-11, "I_sc_ref": 6.43, "N_s": 72, "Name": "SunPower SPR-E20-245", "PTC": 225.5, "R_s": 0.428, "R_sh_ref": 459.56, "T_NOCT": 50.3, "Technology": 7, "V_mp_ref": 40.5, "V_oc_ref": 48.8, "Version": 4, "a_ref": 1.8564, "alpha_sc": 0.004501, "beta_oc": -0.17373, "gamma_r": -0.324, "id": 18010, "resource_uri": "/api/v1/cecmodule/18010/"},
  {"A_c": 1.244, "Adjust": 5.045, "BIPV": false, "Date": "2013-04-02", "I_L_ref": 6.438, "I_mp_ref": 6.05, "I_o_ref": 5.02e-12, "I_sc_ref": 6.43, "N_s": 72, "Name": "SunPower SPR-E20-245-A-AC", "PTC": 225.5, "R_s": 0.467, "R_sh_ref": 390.64, "T_NOCT": 50.3, "Technology": 7, "V_mp_ref": 40.5, "V_oc_ref": 48.8, "Version": 4, "a_ref": 1.7516, "alpha_sc": 0.002508, "beta_oc": -0.12395, "gamma_r": -0.324, "id": 18011, "resource_uri": "/api/v1/cecmodule/18011/"}, 
... truncated ...
{"A_c": 1.631, "Adjust": 23.67, "BIPV": false, "Date": "2013-05-01", "I_L_ref": 6.468, "I_mp_ref": 5.98, "I_o_ref": 9.42e-11, "I_sc_ref": 6.46, "N_s": 96, "Name": "SunPower SPR-E20-327-COM-T5", "PTC": 301.4, "R_s": 0.365, "R_sh_ref": 283.09, "T_NOCT": 46.0, "Technology": 7, "V_mp_ref": 54.7, "V_oc_ref": 64.9, "Version": 4, "a_ref": 2.6047, "alpha_sc": 0.003988, "beta_oc": -0.17698, "gamma_r": -0.386, "id": 18016, "resource_uri": "/api/v1/cecmodule/18016/"}, 
{"A_c": 2.162, "Adjust": 8.562, "BIPV": false, "Date": "2013-01-14", "I_L_ref": 6.434, "I_mp_ref": 5.97, "I_o_ref": 5.28e-10, "I_sc_ref": 6.43, "N_s": 128, "Name": "SunPower SPR-E20-435-COM", "PTC": 400.2, "R_s": 0.252, "R_sh_ref": 456.33, "T_NOCT": 44.6, "Technology": 7, "V_mp_ref": 72.9, "V_oc_ref": 85.6, "Version": 4, "a_ref": 3.6905, "alpha_sc": 0.004501, "beta_oc": -0.30474, "gamma_r": -0.424, "id": 18017, "resource_uri": "/api/v1/cecmodule/18017/"},
{"A_c": 2.162, "Adjust": 5.043, "BIPV": false, "Date": "2014-03-03", "I_L_ref": 6.507, "I_mp_ref": 6.04, "I_o_ref": 1.1e-10, "I_sc_ref": 6.5, "N_s": 128, "Name": "SunPower SPR-E20-440-COM", "PTC": 404.9, "R_s": 0.478, "R_sh_ref": 435.75, "T_NOCT": 44.6, "Technology": 7, "V_mp_ref": 72.9, "V_oc_ref": 86.5, "Version": 4, "a_ref": 3.4917, "alpha_sc": 0.001255, "beta_oc": -0.28199, "gamma_r": -0.424, "id": 18018, "resource_uri": "/api/v1/cecmodule/18018/"},
{"A_c": 1.244, "Adjust": 5.044, "BIPV": false, "Date": "2015-04-03", "I_L_ref": 6.438, "I_mp_ref": 6.05, "I_o_ref": 5.02e-12, "I_sc_ref": 6.43, "N_s": 72, "Name": "Sunpower SPR-E20-245-B-AC", "PTC": 225.5, "R_s": 0.467, "R_sh_ref": 390.63, "T_NOCT": 50.3, "Technology": 7, "V_mp_ref": 40.5, "V_oc_ref": 48.8, "Version": 4, "a_ref": 1.7516, "alpha_sc": 0.002508, "beta_oc": -0.12395, "gamma_r": -0.324, "id": 18391, "resource_uri": "/api/v1/cecmodule/18391/"}]}

wholmgren · 2018-04-03T15:48:30Z

@cwhanse up to you on if we should hold off for the SAM team to update the files. In any case, you'll need to git rm the 2015 files, then commit the change.

cwhanse · 2018-04-06T13:33:27Z

I talked with NREL, we should edit the SAM libraries and move ahead with our merge. The alternative is to wait for the next SAM release, since that is when the SAM libraries are updated.

markcampanelli · 2018-04-06T13:44:47Z

@cwhanse Thanks for working thru the issues with the new version. FWIW, while I was "scraping" the existing cec module dataset for single cells (i.e., Ns=1), my scraper code caught a few entries with missing/bad data.

wholmgren · 2018-04-06T16:17:12Z

@cwhanse and @thunderfish24: do you want to remove those modules too or go ahead and merge? They don't seem to cause a problem for the import function, so my vote is to move on.

cwhanse · 2018-04-06T19:06:05Z

I’d keep any module which has a full set of parameter values. We don’t have a system to judge good from bad parameter sets, so we have no basis to remove a module. Even when a module has a questionable value (e.g. negative resistance, diode factor < 1) the calculation could still produce usable IV curves.

…

Sent from my iPhone On Apr 6, 2018, at 10:17 AM, Will Holmgren <[email protected]<mailto:[email protected]>> wrote: @cwhanse<https://github.com/cwhanse> and @thunderfish24<https://github.com/thunderfish24>: do you want to remove those modules too or go ahead and merge? They don't seem to cause a problem for the import function, so my vote is to move on. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#444 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AFJNL2YmrORSUZIcAzGOE5qVNMpTXWd-ks5tl5UIgaJpZM4TCLDo>.

mikofski · 2018-04-06T19:18:19Z

FYI: pvfree parses the SAM libraries (CEC modules, CEC/SNL inverters, and SNL modules), converts everything to a format suitable for pvlib, and drops incomplete or abnormal records. It also enforces database integrity and validation conditions. The source is open and online here. Don't like the way it's parsed, the search filters, or the datatypes, then contribute. Want some data analysis or validation, like efficiency or IV curves, then we can add them too. EG: here's what SNL modules show:

wholmgren · 2018-04-06T19:44:24Z

The only outstanding issue is to add an entry to the 0.5.2.rst whatsnew file. I don't think there should be any problem with merge commits because git is set to union those files by default.

cwhanse · 2018-04-09T15:02:06Z

@mikofski thanks for that illustration. I'm not opposed to adopting parts of pvfree into pvlib, as a replacement for the current method of reading csv files of model parameters. But we'd want to agree that the change is needed and is an improvement. I don't see that pvlib is ready for this change. The problem is providing a traceable and stable input files. The CEC databases contain module and inverter measurements, not model parameters. The SAM libraries are output of processing the public CEC databases using tools that are now in the SAM SDK, but using those tools to produce the library files is currently somewhat ad hoc. Since we can't produce the model parameters natively in pvlib by processing the CEC databases, we rely on a particular SAM release to provide traceable and stable inputs. Because of this reliance on SAM, I think we're better using the files directly, than we would be with an additional layer of code between the files and pvlib.

I hope this makes sense.

cwhanse · 2018-04-09T20:40:23Z

Note added for whatsnew

wholmgren · 2018-04-09T20:48:35Z

Hm, I don't see the changes to whatsnew. Maybe missed a commit or push step?

wholmgren · 2018-04-09T21:01:15Z

There it is. I don't understand why there is a merge conflict. I tested it locally and it worked fine on my machine. Irritating.

Can you click on the resolve conflicts button and remove the three marked lines?

wholmgren · 2018-04-09T21:44:37Z

Great, thanks!

cedricleroy · 2018-05-07T18:01:19Z

I think f892664 did overwrite a couple of previous things in v0.5.2.rst

There is a "0" here: f892664#diff-2df8cbc501f75a90135963ace8a28f63R1

And the text here has been removed: f892664#diff-2df8cbc501f75a90135963ace8a28f63L15

mikofski · 2018-08-03T22:27:09Z

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

+* Updated libraries for CEC module parameters to SAM release 2017.9.5 
+(library dated 2017.6.30) and CEC inverter parameters to file posted to 
+www.github.com/NREL/SAM on 2018.3.18, with one entry removed due to a 
+missing parameter value. (:issue:'440')


please change single quotes around #440 to backticks

cwhanse added 2 commits March 30, 2018 15:45

Update SAM libraries

799798c

Add new SAM libraries

05d7781

cwhanse added 2 commits March 30, 2018 22:04

Update pvsystem.py

4f780c5

Update pvsystem.py

a48afdd

remove bad inverter line

037755f

update example module output

0da278b

Merge pull request #3 from wholmgren/samupdate

ce210b5

remove bad inverter line, update test

wholmgren mentioned this pull request Mar 30, 2018

update sam library files #443

Closed

5 tasks

Adding LinkeTurbidities.h5

2751a43

remove old SAM libraries

150cb47

wholmgren mentioned this pull request Apr 6, 2018

Update adrinverter parameter database #317

Closed

wholmgren added this to the 0.5.2 milestone Apr 6, 2018

wholmgren added the enhancement label Apr 6, 2018

adding note to whatsnew for update to SAM libraries

4668142

cwhanse added 3 commits April 9, 2018 15:13

Update v0.5.2.rst

375d030

Update v0.5.2.rst

e2a1e85

Merge branch 'master' into samupdate

9376f85

wholmgren merged commit f892664 into pvlib:master Apr 9, 2018

wholmgren mentioned this pull request May 9, 2018

fix whatsnew problems #454

Closed

mikofski reviewed Aug 3, 2018

View reviewed changes

cwhanse deleted the samupdate branch September 21, 2018 20:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update SAM libraries #444

Update SAM libraries #444

cwhanse commented Mar 30, 2018 •

edited

Loading

wholmgren commented Mar 30, 2018

cwhanse commented Mar 30, 2018 via email

cwhanse commented Mar 30, 2018

wholmgren commented Mar 30, 2018

wholmgren commented Mar 30, 2018

cwhanse commented Mar 30, 2018

cwhanse commented Mar 30, 2018

mikofski commented Mar 31, 2018

wholmgren commented Apr 3, 2018

cwhanse commented Apr 6, 2018

markcampanelli commented Apr 6, 2018

wholmgren commented Apr 6, 2018

cwhanse commented Apr 6, 2018 via email

mikofski commented Apr 6, 2018

wholmgren commented Apr 6, 2018

cwhanse commented Apr 9, 2018

cwhanse commented Apr 9, 2018

wholmgren commented Apr 9, 2018

wholmgren commented Apr 9, 2018

wholmgren commented Apr 9, 2018

cedricleroy commented May 7, 2018

mikofski Aug 3, 2018

Update SAM libraries #444

Update SAM libraries #444

Conversation

cwhanse commented Mar 30, 2018 • edited Loading

pvlib python pull request guidelines

wholmgren commented Mar 30, 2018

cwhanse commented Mar 30, 2018 via email

cwhanse commented Mar 30, 2018

wholmgren commented Mar 30, 2018

wholmgren commented Mar 30, 2018

cwhanse commented Mar 30, 2018

cwhanse commented Mar 30, 2018

mikofski commented Mar 31, 2018

wholmgren commented Apr 3, 2018

cwhanse commented Apr 6, 2018

markcampanelli commented Apr 6, 2018

wholmgren commented Apr 6, 2018

cwhanse commented Apr 6, 2018 via email

mikofski commented Apr 6, 2018

wholmgren commented Apr 6, 2018

cwhanse commented Apr 9, 2018

cwhanse commented Apr 9, 2018

wholmgren commented Apr 9, 2018

wholmgren commented Apr 9, 2018

wholmgren commented Apr 9, 2018

cedricleroy commented May 7, 2018

mikofski Aug 3, 2018

Choose a reason for hiding this comment

cwhanse commented Mar 30, 2018 •

edited

Loading