Add function to fit Sandia inverter model #1011

cwhanse · 2020-07-24T14:58:25Z

Closes Implement a function to calculate inverter model parameters #975
I am familiar with the contributing guidelines
Tests added
Updates entries to docs/sphinx/source/api.rst 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 and Milestone are assigned to the Pull Request and linked Issue.

Inputs are: efficiency curves with DC voltage and AC power, rated AC power and night tare. Returns dict ready for the Sandia inverter model function. The algorithm reference is on the PVPMC site.

…nverter_fit

docs/sphinx/source/api.rst

pvlib/inverter.py

kandersolar · 2020-08-03T18:25:53Z

pvlib/inverter.py

+        Columns must be ``'fraction_of_rated_power'``, ``'dc_voltage_level'``,
+        ``'dc_voltage'``, ``'ac_power'``, ``'efficiency'``. See notes for the
+        definition and unit for each column.
+    p_ac_0 : numeric


Is it intentional that p_ac_0 and p_nt are spelled differently here than in inverter.sandia? Seems like these two functions would benefit from a consistent interface.

I agree, which would you prefer? p_ac_0 is more appealing than Pac0 as a program variable. The inertia is with Pac0, which is the column key for this parameter read from the SAM files.

It's not clear to me where the balance should be struck between (1) presenting a consistent interface across functions or (2) maintaining as clear a connection as possible to the original model definitions. I think I would lean towards p_ac_0 over Pac0 with the hope that existing Pac0s would be changed to match.

That is my preference as well. The function returns a dict ready for pvlib.inverter.sandia so at least in one direction the two are compatible.

pvlib/inverter.py

pvlib/tests/test_inverter.py

kandersolar · 2020-08-03T19:41:40Z

pvlib/inverter.py

+    Parameters
+    ----------
+    curves : DataFrame
+        Columns must be ``'fraction_of_rated_power'``, ``'dc_voltage_level'``,


Is fraction_of_rated_power actually required? I don't see it getting used anywhere.

The values are not used explicitly, but I think it would be confusing to describe the needed input without fraction_of_rated_power. If working from a datasheet, efficiency in usually given in terms of fraction_of_rated_power rather than ac_power.

cwhanse · 2020-08-05T15:21:35Z

Test failures are due to #1018

kandersolar

LGTM. Idea for a future PR: I think pairing the fit functions with the associated models would make for good gallery examples.

…nverter_fit

cwhanse · 2020-08-12T16:51:25Z

@Peque any comments or edits to this before we merge?

wholmgren · 2020-08-12T18:43:47Z

I'm not going to make much use of this function so my comments come from an API design/consistency point of view...

I generally want to avoid DataFrame input to functions because the API should be as self-documenting as possible. Here there's already an issue with an unused column that, to me, is a red flag for the API design. I also recommend refactoring a lot of the logic into a function that works on one curve at a time curve.

cwhanse · 2020-08-12T21:24:59Z

I think a Dataframe is a convenient way for a user to organize the data input, but I'm open to different ideas.

I can remove the fraction_of_rated_power column, which isn't used by this code. I thought to include it as a convenience for a user who is preparing the data input from the table typically found in a datasheet. The rated power levels are an index in that table, without the values in the Dataframe it's less easy to see if you have omitted a point, or to check that the values are typed correctly.

The data are a set of efficiency curves, where a curve is set of points (efficiency, ac power) (the docstring is incorrect on this point and I'll fix that). The curves do not have to be, but usually are, aligned on a set of common efficiency values. The algorithm needs to know one fact about each curve, the DC voltage level. If working from a datasheet there are typically 3 curves, one at each voltage level. From lab data, several curves are measured at each voltage level, and the algorithm needs to accommodate these repeated measurements.

Data = a list of tuples (voltage level, curve) where curve is a 2d array? May be awkward both for the user and for this function.

The algorithm doesn't work one curve at a time. The parameters are gotten from regressions across DC voltage levels (i.e., across subsets of the curves). Here's a diagram that illustrates how the 6 parameters are gotten from the set of curves. Three coefficients come from fitting a quadratic to the curve(s)

wholmgren · 2020-08-12T21:46:46Z

pvlib/inverter.py

+    # independent variable for regressions, x_d
+    x_d = v_d - v_nom
+
+    curves['dc_power'] = curves['ac_power'] / curves['efficiency']


Doesn't this modify the input DataFrame?

wholmgren · 2020-08-12T22:00:06Z

Sorry I now see I misread the loop and assumed something about the structure of the rows of the DataFrame that is not true.

I think a Dataframe is a convenient way for a user to organize the data input

Agreed, but the same is true for many of the other low level functions in pvlib and they still require arguments for API clarity. ModelChain takes DataFrame input and we see a lot more user confusion there about what the input needs to contain (granted it's a confusing thing for many reasons).

Apologies if I'm still missing something, but is there a reason to avoid an api like voltage_level, dc_voltage, efficiency, ac_power where each is an array? Of course they can be Series if the user starts with a DataFrame.

I can remove the fraction_of_rated_power column, which isn't used by this code. I thought to include it as a convenience for a user who is preparing the data input from the table typically found in a datasheet. The rated power levels are an index in that table, without the values in the Dataframe it's less easy to see if you have omitted a point, or to check that the values are typed correctly.

This doesn't sound like the function's problem to solve. Perhaps would be good to include in an example in the gallery.

cwhanse · 2020-08-13T15:38:15Z

Apologies if I'm still missing something, but is there a reason to avoid an api like voltage_level, dc_voltage, efficiency, ac_power

No you aren't missing something, that's a good idea - easy for a user to get to those inputs if a datasheet table is transcribed to a Dataframe, and makes the inputs explicit. Thanks.

wholmgren

one nit, but otherwise LGTM assuming the tests pass when the syntax error is fixed.

wholmgren · 2020-08-13T21:50:01Z

pvlib/inverter.py

+    ac_power : Series
+        AC power output at each data point [W].
+    dc_power : Series
+        DC power input at each data point [W].
+    dc_voltage : Series
+        DC input voltage at each data point [V].
+    dc_voltage_level : Series


nit: these should be array_like, not Series.

Peque · 2020-08-14T11:04:15Z

@cwhanse Looks great. 😊

cwhanse added 8 commits June 22, 2020 08:18

WIP: fitting inverter model

7de7a77

correct calculated data, improve test

31f0407

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

4426306

…nverter_fit

move code to pvlib modules, rename data file

0391eb0

fix test, stickler

e7c1594

sort out np.polyfit vs. np.polynomial.polynomial.polyfit

c38679f

correct file name

b67766d

api, whatsnew

c0f96dd

cwhanse added the enhancement label Jul 24, 2020

cwhanse added this to the 0.8.0 milestone Jul 24, 2020

docstring improvements

3b37893

kandersolar reviewed Aug 3, 2020

View reviewed changes

cwhanse added 5 commits August 4, 2020 12:24

revisions from review

ae4380f

correct use of np.roots

ecce111

use pytest.approx instead of converting dicts to np.array

303ab1c

correct kwarg for pytest.approx

768d39d

back to solve_quad instead of np.roots, lint

e8662fb

kandersolar approved these changes Aug 6, 2020

View reviewed changes

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

a9bd4bf

…nverter_fit

wholmgren reviewed Aug 12, 2020

View reviewed changes

change to Series inputs

de6916c

wholmgren reviewed Aug 13, 2020

View reviewed changes

correct test, Series to array_like

3d38e6c

cwhanse merged commit 49da031 into pvlib:master Aug 14, 2020

cwhanse mentioned this pull request Aug 20, 2020

Keys described in snlinverter are not what is used in code #976

Closed

cwhanse deleted the inverter_fit branch October 27, 2020 21:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add function to fit Sandia inverter model #1011

Add function to fit Sandia inverter model #1011

cwhanse commented Jul 24, 2020 •

edited

Loading

kandersolar Aug 3, 2020

cwhanse Aug 4, 2020

kandersolar Aug 4, 2020

cwhanse Aug 4, 2020

kandersolar Aug 3, 2020

cwhanse Aug 4, 2020 •

edited

Loading

cwhanse commented Aug 5, 2020

kandersolar left a comment

cwhanse commented Aug 12, 2020

wholmgren commented Aug 12, 2020

cwhanse commented Aug 12, 2020

wholmgren Aug 12, 2020

wholmgren commented Aug 12, 2020

cwhanse commented Aug 13, 2020

wholmgren left a comment

wholmgren Aug 13, 2020

Peque commented Aug 14, 2020

Add function to fit Sandia inverter model #1011

Add function to fit Sandia inverter model #1011

Conversation

cwhanse commented Jul 24, 2020 • edited Loading

kandersolar Aug 3, 2020

Choose a reason for hiding this comment

cwhanse Aug 4, 2020

Choose a reason for hiding this comment

kandersolar Aug 4, 2020

Choose a reason for hiding this comment

cwhanse Aug 4, 2020

Choose a reason for hiding this comment

kandersolar Aug 3, 2020

Choose a reason for hiding this comment

cwhanse Aug 4, 2020 • edited Loading

Choose a reason for hiding this comment

cwhanse commented Aug 5, 2020

kandersolar left a comment

Choose a reason for hiding this comment

cwhanse commented Aug 12, 2020

wholmgren commented Aug 12, 2020

cwhanse commented Aug 12, 2020

wholmgren Aug 12, 2020

Choose a reason for hiding this comment

wholmgren commented Aug 12, 2020

cwhanse commented Aug 13, 2020

wholmgren left a comment

Choose a reason for hiding this comment

wholmgren Aug 13, 2020

Choose a reason for hiding this comment

Peque commented Aug 14, 2020

cwhanse commented Jul 24, 2020 •

edited

Loading

cwhanse Aug 4, 2020 •

edited

Loading