Update adrinverter parameter database

[adriesse]

• Closes Update adrinverter parameter database #317
• 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.

The previous ADR inverter parameter database files were generated using the original lab measurements published by the CEC and the resulting parameters would occasionally predict slightly different AC power compared to the Sandia model and parameters derived from the same measurements. The question of which was better was not that important because the differences were small.

Since the CEC no longer publishes those measurements, this update simply fits the ADR model to the Sandia model output and the differences in AC output power are now even smaller. With the ADR coefficients rounded to 5 decimal places (so they don't look too silly) the absolute worst case difference in AC output power between the two models among all matrix points and all inverters is approximately 50 ppm.

[adriesse]

Could someone give me a hint how to update test_pvsystem.py? It's not clear to me where the adr model parameters are coming from...

[kandersolar]

I think this is the relevant line for the test failure: https://github.com/adriesse/pvlib-python/blob/adr_db_update/pvlib/tests/test_modelchain.py#L139

Do we have a policy/convention for whether changing entry names in these databases constitutes a breaking change as it relates to semver? I'm wondering if this needs to wait for 0.10.0.

[adriesse]

Do we have a policy/convention for whether changing entry names in these databases constitutes a breaking change as it relates to semver? I'm wondering if this needs to wait for 0.10.0.

@pvlib/pvlib-maintainer this seems like a call for opinions.

I could append the new data to the old to make it non-breaking.

[cwhanse]

As far as changing the keys (names) in the data files, my view: we copy from SAM who imports from the CEC. SAM constructs the product key from the manufacturer and product strings that the CEC publish, which in turn are submitted to the CEC by manufacturers and test labs. As a consequence, product keys change in the SAM file. Until pvlib (and SAM) can unhitch from the CEC's list (which is maintained for different purposes), managing changes in key names is out of our hands. I think the best we can do is to keep some consistency between pvlib and SAM.

As far as appending old data to a new file: I think this is a good idea provided we can screen to avoid having duplicate entries.

[kandersolar]

managing changes in key names is out of our hands

Managing the changes is not entirely out of our hands; we have the option to at least delay distributing database updates in pvlib.

What's not clear to me is whether database keys should be considered part of the pvlib API. On the one hand, distributing tables with changed/removed keys seems as breaking a change as any we make, in that it turns working code into non-working code. On the other hand we know that these tables are going to continue evolving and I don't think the eventual pvlib 1.0 should have to increment its version number from 1.x to 2.x just to allow an update of the CEC module database.

[adriesse]

Pending other inputs I would then tend toward the append solution. There will be no duplicate records.

[adriesse]

Append complete.

[adriesse]

Any more volunteers to review this one? Only one line of code!

[cwhanse]

@adriesse if we merge this, can we anticipate having a fit_adr function added in the near future? Since the parameter set originates here, I feel like we should offer some ability to reproduce or verify.

[adriesse]

Here is a link to a fitting function and more:

https://github.com/adriesse/pvpltools-python/blob/master/pvpltools/power_conversion.py

Fitting is basically just throwing the matrix data at scipy curvefit.
To verify a set of parameters, just create a matrix of inputs, run both models and compare.

[kandersolar]

I think some rows from the older table might have gotten lost in the append, perhaps due to duplicate Name entries. Check for example Name=='Dow Chemical: 240V [CEC 2011]'. Also some columns (Source, Vintage, TambLow, TambHi, Weight) are all blank in the new rows, is that intentional?

[adriesse]

The old table had 12 rows with duplicated keys, which I dropped. The Dow Chemical one appeared four times actually. The information for the additional fields is unfortunately not available in my source, the Sandia inverter model parameter database, so the blank fields are indeed sort of intentional.

[kandersolar]

The Names are duplicated, but the parameters (at least for the 4x Dow) are not identical, so I'd lean towards keeping them if possible since all four get returned by pvlib.pvsystem.retrieve_sam('adrinverter') with pvlib 0.9.4.

[adriesse]

I asked myself what anyone would ever do with these duplicates keys except swear at them when the usual syntax for extracting a single set of parameters failed, and drew a blank. If I was actually tempted to delete the fourth one as well. But maybe there are other angles I haven't considered.

[kandersolar]

Fair. I was thinking more about backwards compatibility than future utility, but I won't harp on it. Should we delete the older file and save 300 kB? Otherwise I think LGTM.

can we anticipate having a fit_adr function added in the near future?

Tracked in #1309

[adriesse]

We could keep the old file for a bit, kind of like a deprecation cycle. But I'm not particularly attached to it.

[kandersolar]

No objection from me. I'd lean towards deleting the old file but it's not a blocker.

@adriesse FYI there's a merge conflict

[adriesse]

I'd lean towards deleting the old file

Any one else leaning one way or another? I'm pretty comfortable on the fence.

[cwhanse]

I would delete the old file. We don't keep old versions of the files from SAM.

[kandersolar]

Thanks @adriesse!

[adriesse]

One less item on my bucket list!