port pvl_adrinverter #288

VolkrB · 2017-01-04T21:23:14Z

#160 In short:

Ported mat -> csv adrinverter data file

pvsystem.py

included import of csv in pvsystem.py retrieve_sam function
added adrinverter function (two lines are a bit ugly, because i am storing coefficients in a string instead of making a column for each)

modelchain.py

added adrinverter function to wrapper

test_modelchain.py

removed xfail for adrinverter
added tests using csv data (Zigor inverter)
(had to also change the single diode test, since i replaced cecinverter with adrinverter)

test_pvsystem.py

added test for adrinverter function

Need to document after everything looks good

I've compared inverter data from the adrinverter function to the sandiainverter function to see if, at least, the results are of the same magnitude and follow a similar trend. They do, however would be nice to see results from the matlab version to test against.

let me know what i can change/improve etc

wholmgren

@VolkrB thanks for this great PR. Please see the line comments for more.

wholmgren · 2017-01-05T02:23:20Z

pvlib/pvsystem.py

@@ -1122,6 +1125,9 @@ def retrieve_sam(name=None, path=None):
        elif name == 'sandiamod':
            csvdata = os.path.join(
                data_path, 'sam-library-sandia-modules-2015-6-30.csv')
+        elif name == 'adrinverter':
+            csvdata = os.path.join(


looks like this might fit on a single line that's less than 80 characters.

wholmgren · 2017-01-05T02:28:50Z

pvlib/pvsystem.py

+def adrinverter(v_dc, p_dc, inverter):
+
+    ce_clean = [','.join(inverter['ADRCoefficients'][1:-1].split())]
+    ce_list = np.array([float(s) for s in ce_clean[0].split(',')])


inverter['ADRCoefficients'] should be parsed at import time

Yea, seems like that's where it should be

wholmgren · 2017-01-05T02:32:03Z

pvlib/test/test_modelchain.py

@@ -33,8 +33,8 @@ def cec_dc_snl_ac_system(sam_data):
    module_parameters['b'] = 0.05
    module_parameters['EgRef'] = 1.121
    module_parameters['dEgdT'] = -0.0002677
-    inverters = sam_data['cecinverter']
-    inverter = inverters['ABB__MICRO_0_25_I_OUTD_US_208_208V__CEC_2014_'].copy()
+    inverters = sam_data['adrinverter']


maybe I'm forgetting something, but don't we want a new test here instead of changing the existing test?

on a second read, maybe this is ok because all of the methods are still being tested in some way. Is that right?

Yea they are, I think the coverage is the same- this was something I was debating as well.

ok with me to leave it alone then

I'm only now realizing that this change to the cec_dc_snl_ac_system fixture makes the name no longer correct. I do think it would be worthwhile to make a second test called cec_dc_adr_ac_system or something like it. Then you can revert the change to the test_dc_models fixture, while also updating the test_ac_models ac_systems dictionary.

wholmgren · 2017-01-05T02:36:51Z

pvlib/data/adr-library.csv

@@ -0,0 +1,1762 @@
+Name,Manufacturer,Model,Source,Vac,Vintage,Pacmax,Pnom,Vnom,Vmin,Vmax,ADRCoefficients,Pnt,Vdcmax,Idcmax,MPPTLow,MPPTHi,TambLow,TambHi,Weight
+0,0,0,0,0.0,0.0,0.0,0,0,0,0,0,0.0,0.0,0.0,0.0,0.0,0.0,0.0,0.0


are these 2 lines of 0s here because of the _parse_raw_sam_df function's skip_rows argument? I think we should keep this file clean and make the pvlib parser smarter. I don't remember the logic for that parser (I don't even remember if I wrote it).

Yea exactly. I could write a conditional statement in _parse_raw_sam_df to only skip rows for certain csvs, but maybe you can think of something better than that?

I'm struggling to think of a good solution for this and the array parsing issue. One relatively simple option is to replace the 0 lines with metadata lines like in the sam files. So, we'd want a units line and then I guess a copy of the column names. Or maybe just empty cells instead of 0s would be better? The end of the parser function could have a conditional on the presence of 'ADRCoefficients' for the array parsing issue discussed above.

VolkrB · 2017-01-05T21:55:31Z

Updated the csv and moved inverter['ADRCoefficients'] parsing to pvsystem._parse_raw_sam_df

VolkrB · 2017-01-05T23:56:15Z

pvlib/pvsystem.py

+                                          np.array([float(s) for s in
+                                                    [','.join(x[1:-1]
+                                                              .split())]
+                                                    [0].split(',')]))


I realize this is a pretty hackie, ugly, line of code. Trying to see if I can get this to be more elegant.

I think this works: lambda x: list(map(float, x.strip(' []').split(' ')))

not sure if there are pros/cons to adding np.array around the list.

also, you don't have to stay justified with the opening parenthesis. you can make only a 4 space indent on the line below the opening parenthesis. that alone would help readability!

I'd get rid of the empty else block, too.

other than that, I think you only need to add documentation and then we'll be set.

Maybe a code comment should go here to explain what this line is supposed to do. Does is really all have to be done in one line?

adriesse · 2017-01-09T14:54:11Z

pvlib/pvsystem.py

+                     pdc**2*(1./vdc-1)])
+
+    p_loss = np.dot(ce_list, poly)
+    ac_power = p_nom * (pdc-p_loss)


the name "ac_power" seems to be breaking the naming pattern...

adriesse · 2017-01-09T15:31:33Z

I reviewed the code and didn't see any mistakes. Since all the coefficients are normalized, it would pretty easy to create some synthetic test cases. For example, each coefficient with a value of 4% produces a loss of Pnom * 4% at Pdc==Pnom and Vdc==Vnom. Depending on which particular coefficient it is, this loss will vary with Pdc, Vdc, both, or neither.

VolkrB · 2017-01-09T17:33:52Z

I cleaned up that big ugly parsing line based on @wholmgren suggestions.

@adriesse: I always struggle with variable naming, so perhaps you, @wholmgren and I can all agree on something. Also, could you review my pushed documentation and suggest proper definition, references, etc? (i guess this would also be a good place to discuss the naming of vars)

adriesse · 2017-01-09T21:44:10Z

The shorter parsing line is definitely easier to parse, but it still wouldn't hurt to throw a comment in there which would be even easier to parse for readers.

I see that the ac_power name came from the other inverter function and is used in matlab. I was anticipating something like pac or p_ac, but there doesn't seem to be a risk of misunderstanding so I don't mind if you leave it.

Can you just copy the documentation from the matlab version?

VolkrB · 2017-01-10T18:01:28Z

Updated docs/whatsnew. Let me know if I need to add/change etc.

wholmgren

Can you double check the indentation and line length of the documentation? flake8 might help here. I think p_dc is a little off.

wholmgren · 2017-01-10T18:22:36Z

pvlib/pvsystem.py

@@ -1059,6 +1059,7 @@ def retrieve_sam(name=None, path=None):
        * CEC module database
        * Sandia Module database
        * CEC Inverter database
+        * Antone Driesse Inverter database 


(Sorry Anton)

wholmgren · 2017-01-10T18:23:18Z

pvlib/pvsystem.py

@@ -2048,30 +2051,40 @@ def snlinverter(v_dc, p_dc, inverter):

 def adrinverter(v_dc, p_dc, inverter):
    r'''
-    Converts DC power and voltage to AC power using Sandia's
-    Grid-Connected PV Inverter model.
+    PVL_ADRINVERTER Converts DC power and voltage to AC power


no PVL_ADRINVERTER

wholmgren · 2017-01-10T18:24:29Z

pvlib/pvsystem.py


    inverter : dict-like
-        A dict-like object defining the inverter to be used.
+        A struct defining the inverter to be used, giving the


wholmgren · 2017-01-10T18:24:43Z

pvlib/pvsystem.py

-        A dict-like object defining the inverter to be used.
+        A struct defining the inverter to be used, giving the
+        inverter performance parameters according to the model
+        developed by Anton Driesse[1].


space before [1]

wholmgren · 2017-01-10T18:26:14Z

pvlib/pvsystem.py

        A set of inverter performance parameters are provided with
-        pvlib, or may be generated from the library using retrievesam. 
+        pvlib, or may be generated from the library using retrievesam.


maybe something like "loaded from the supplied data table using retrieve_sam" would be more accurate.

wholmgren · 2017-01-10T18:27:41Z

pvlib/pvsystem.py

        See Notes for required keys.

    Returns
    -------
    ac_power : numeric
-        Modeled AC power output given the input DC voltage, v_dc, and
-        input DC power, p_dc.
+        A numpy array of modeled AC power output given the input


"A numpy array" is not quite right. It returns a Series if the input data is also a Series.

wholmgren · 2017-01-10T18:36:02Z

pvlib/pvsystem.py

@@ -1178,7 +1180,8 @@ def _parse_raw_sam_df(csvdata):
    df = df.transpose()
    if 'ADRCoefficients' in df.index:
        ad_ce = 'ADRCoefficients'
-        df.loc[ad_ce] = df.loc[ad_ce].map(lambda x:list(
+        # parses string into list containing floats:


# for each inverter, parses a string of coefficients like # ***insert example*** # into a list containing floats

adriesse · 2017-01-10T21:24:32Z

pvlib/pvsystem.py

@@ -1122,6 +1127,8 @@ def retrieve_sam(name=None, path=None):
        elif name == 'sandiamod':
            csvdata = os.path.join(
                data_path, 'sam-library-sandia-modules-2015-6-30.csv')
+        elif name == 'adrinverter':
+            csvdata = os.path.join(data_path, 'adr-library.csv')


The parameters in this particular file were generated using the CEC efficiency measurements available to me as of 2013-10. It might be good to add the date suffix. I'm looking into doing an update.

ok I will add a date suffix

adriesse · 2017-01-10T21:30:30Z

pvlib/pvsystem.py

+    p_loss = np.dot(np.array(ce_list), poly)
+    ac_power = p_nom * (pdc-p_loss)
+
+    p_nt = -1*np.absolute(p_nt)


Earlier versions of the cec/sam/sandia database were not consistent: most inverters had positive p_nt but many were negative. The latest sam database I looked at had only positve values.

so p_nt = -1*np.absolute(p_nt) should just be -1*p_nt ?

That was just background info. The absolute() shouldn't be needed, but makes it more robust.

VolkrB · 2017-01-17T18:52:29Z

I ran flake8 and added a date suffix

wholmgren · 2017-01-17T19:55:22Z

Looks good to me. We can ignore the failing travis builds.

@adriesse -- you get to make the final stamp of approval since your name is on it!

adriesse · 2017-01-17T21:22:23Z

pvlib/test/test_pvsystem.py

+
+    pacs = pvsystem.adrinverter(vdcs, pdcs, inverters[testinv])
+    assert_series_equal(pacs, pd.Series([-0.25000, 105.635043, 503.769061]))
+


This test may catch some types of future bugs, but it's not a good PV example. The current, voltage and power ranges are way too small for this Zigor model inverter.

Another idea might be to use some voltage and current combinations found in the pdf file on the CEC/gosolar website and look for the results of the model to be approxiamtely equal.

adriesse · 2017-01-17T21:34:54Z

Other than the test everything looks good to me.

VolkrB · 2017-01-17T21:35:52Z

I don't mind updating the test. I've found a nice reference on gosolar and working on it now

VolkrB · 2017-01-17T23:48:16Z

@adriesse could you comment on the changes to the test when you get a chance?

adriesse · 2017-01-18T09:53:55Z

Sure. So this is indeed a more realistic set of test conditions. Did you calculate the expected outputs using an independent method, e.g. Excel?

Unfortunately you dropped the case that tests the night-time tare functionality and this got me thinking that the clipping condition isn't tested either. I guess this wouldn't show up in the code coverage metrics since it is done with a "where" rather than "if", but it still seems like a good idea to include them.

A test case with scalar inputs, like for pvl_snlinverter, would be good as well.

We could stop here, or we could go further--but that might lead to changes to the inverter code. What should happen when Pdc or Vdc is less than zero? There are in fact many realistic and non-realistic combinations of Pdc and Vdc where the model would not produce a realistic output, i.e. it should not be used to extrapolate beyond the normal operating range of the inverter.

What do you think about producing nan's for out-of-range conditions? This behavior could be triggered by an optional parameter "range_check", or even multiple parameters for different checks. Another option would be to return one of more columns of flags.

VolkrB · 2017-01-19T18:10:13Z

Made changes based on input from @adriesse

adriesse · 2017-01-19T20:40:52Z

pvlib/pvsystem.py

@@ -2051,7 +2051,7 @@ def snlinverter(v_dc, p_dc, inverter):
    return ac_power


-def adrinverter(v_dc, p_dc, inverter):
+def adrinverter(v_dc, p_dc, inverter, vtol=0.10):


vtol : numeric
A unit-less fraction that determines how far the efficiency model is allowed to extrapolate beyond the inverter's normal input voltage operating range. 0.0 <= vtol <= 1.0

adriesse · 2017-01-19T20:46:02Z

I think this is good, Volker. With the added comment for the optional parameter I think we could stop there.

adriesse · 2017-01-19T20:47:05Z

I wonder how many people are actually watching this thread...

jforbess · 2017-01-20T15:41:23Z

I'm reading it. I get all the threads in my email, and read them at some regular frequency.

…

On Thu, Jan 19, 2017 at 12:47 PM, Anton Driesse ***@***.***> wrote: I wonder how many people are actually watching this thread... — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#288 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AH66AYiqDVQgEz8NUuL5OMkXbk2ioAkvks5rT8vJgaJpZM4LbFN8> .

wholmgren · 2017-01-26T18:59:23Z

It's not immediately clear to me if/how the vtol parameter is being tested. Maybe a comment in the test function would be enough. We do need at least one more test that sets the parameter, though.

adriesse · 2017-01-28T10:31:53Z

There is one case with v_dc below the threshold determined by vtol, and another with v_dc above. These both produce nan outputs.

I can image tons of additional tests, and garbage in/out scenarios but don't have a good feel for what's appropriate in this context.

wholmgren · 2017-01-28T17:26:55Z

Thanks for clarifying, Anton. In that case I recommend one additional test that uses the same input but with vtol set sufficiently large so that the returned values are no longer nan. This way we have a test that actually sets the value of the parameter instead of relying on the default value, thus testing both the presence of the keyword and that its value is respected.

adriesse · 2017-01-28T17:56:55Z

That seems a bit like testing the compiler's handling of parameter defaults, doesn't it?!

I have another suggestion. How about putting the inverter parameters in the code rather than reading them from the file? This removes a dependency and also makes clear to the reader what the power and voltage ranges are for the test.

wholmgren · 2017-01-28T18:32:25Z

My primary concern is testing that the function can be called when the keyword is used. This ensures that a test will fail if anyone ever tries to remove vtol or change vtol to something else.

Regarding testing the value of the keyword, I am not at all concerned about the interpreter, but rather I am concerned that the value is actually used later in the code. For example, in prototyping the function there could have been a hardcoded vtol = 0.1 line that was not removed. You would be right to say it should be obvious when reviewing the code, but I make these kinds of mistakes all the time. I am pretty sure that this has happened in pvlib python before. I think it was in singlediode, desoto, or maybe the earlier version of the sapm function.

I like the idea of hardcoding the test parameters for the adrinverter function. That makes me realize that we're not explicitly testing the retrieve_sam function for any of the data types. For this PR, we'd only need to worry about testing it with the 'adrinverter' option.

wholmgren · 2017-02-07T16:39:04Z

Hi @VolkrB, can you merge/rebase your PR and add one more simple test using vtol as described above? Any test that puts vtol in the function call would be fine with me. Testing using hardcoded values and testing retrieve_sam can be implemented later. Thanks!

VolkrB · 2017-02-07T17:06:15Z

Sorry I haven't looked at this in a while. I hope the merge/push looks ok

wholmgren · 2017-02-07T17:24:02Z

There are some errors in the test suite that are related to the combination of changing the default ModelChain inverter and the introduction of the vtol/nans.

VolkrB · 2017-02-07T17:58:57Z

oops. Seems like that test has non-physical results. I put in the appropriate nans in the params

wholmgren

@VolkrB I noticed a couple of relatively minor things while making one last read through. Sorry to have missed these until now. I think it should be quick to finish, though.

wholmgren · 2017-02-07T21:09:57Z

pvlib/pvsystem.py

+             typically the level at which the highest efficiency is achieved,
+             (V).
+
+    pac_max  The maximum AC output power value, used to clip the output if


pac_max and ce_list are wider than this column

should the column header be longer or the variables shorter?

A longer Column header by one = sign. You'll also need to add one space between all of the Column and the Description sections so that the table stays properly formatted. I guess you should probably also make the Description header longer and/or make the text fit within the existing Description header.

I'm pretty sure that Sphinx would build the table in its present form and spit out a warning. It's not a big deal if it's just one table, but every silly warning makes it slightly harder to identify other issues with the documentation. If you actually build the docs and don't see a warning then we can ignore all this.

wholmgren · 2017-02-07T21:10:45Z

pvlib/pvsystem.py

+
+    vtol : numeric
+        A unit-less fraction that determines how far the efficiency model is allowed
+        to extrapolate beyond the inverter's normal input voltage operating range.


these lines are a few characters too long

wholmgren · 2017-02-07T22:16:28Z

pvlib/test/test_modelchain.py

@@ -33,8 +33,8 @@ def cec_dc_snl_ac_system(sam_data):
    module_parameters['b'] = 0.05
    module_parameters['EgRef'] = 1.121
    module_parameters['dEgdT'] = -0.0002677
-    inverters = sam_data['cecinverter']
-    inverter = inverters['ABB__MICRO_0_25_I_OUTD_US_208_208V__CEC_2014_'].copy()
+    inverters = sam_data['adrinverter']


I'm only now realizing that this change to the cec_dc_snl_ac_system fixture makes the name no longer correct. I do think it would be worthwhile to make a second test called cec_dc_adr_ac_system or something like it. Then you can revert the change to the test_dc_models fixture, while also updating the test_ac_models ac_systems dictionary.

VolkrB · 2017-02-14T17:55:10Z

adrinverter is not being tested in the singlediode dc modelchain, but it's being tested in the ac_model chain. Should I write a new test function for including it in the dc modelchain or maybe there's a better solution?

wholmgren · 2017-02-14T18:16:59Z

No need to write a new test function for the dc modelchain. I'm confused about why the 'singlediode' test_dc_models test passes, though. It's supposed to check to the second decimal, but you've (I think inadvertently) changed the value in the first decimal.

VolkrB · 2017-02-14T18:34:53Z

failed for check_less_precise = 3, I updated with the correct result

wholmgren · 2017-02-14T18:41:01Z

Thanks. The test errors are unrelated to this PR, so I think we're ready to merge. Agreed?

VolkrB · 2017-02-14T19:01:44Z

Looks good on my end

adriesse · 2017-02-14T20:07:37Z

Agreed.

wholmgren · 2017-02-14T20:27:24Z

thanks @VolkrB and @adriesse!

VolkrB added 2 commits January 3, 2017 16:50

added adr csv

5de8bb7

ADRinv implemented/tests

745b8db

wholmgren reviewed Jan 5, 2017

View reviewed changes

wholmgren added this to the 0.4.4 milestone Jan 5, 2017

wholmgren added the enhancement label Jan 5, 2017

Added meta data to csv, moved parsing

3861cbe

VolkrB commented Jan 5, 2017

View reviewed changes

adriesse reviewed Jan 9, 2017

View reviewed changes

Cleaned up parsing line, started docs

6b2a3ee

Updated docs/whatsnew

c796e75

wholmgren reviewed Jan 10, 2017

View reviewed changes

VolkrB added 2 commits January 10, 2017 12:27

flake8 docs, fixed typos, other mistakes

9f94353

p_dc

a08c873

adriesse reviewed Jan 10, 2017

View reviewed changes

added date to adr-inv data csv

07f3b1c

updated pvsystem with new adr-inv csv filename

18ec0ea

adriesse reviewed Jan 17, 2017

View reviewed changes

updated adr-inverter test case

a850c3d

adriesse reviewed Jan 19, 2017

View reviewed changes

updated docs with vtol

91bb7fa

VolkrB added 2 commits February 7, 2017 09:50

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

eee4a7d

added vtol=0.20 test

7b7e3ee

changed test params in test_mc to nan

e21ac2e

wholmgren reviewed Feb 7, 2017

View reviewed changes

new adrinv test fixture/formatting

0a6f142

typo in test

716fed0

wholmgren merged commit 3d42536 into pvlib:master Feb 14, 2017

wholmgren mentioned this pull request Feb 14, 2017

port pvl_adrinverter #160

Closed

		@@ -0,0 +1,1762 @@
		Name,Manufacturer,Model,Source,Vac,Vintage,Pacmax,Pnom,Vnom,Vmin,Vmax,ADRCoefficients,Pnt,Vdcmax,Idcmax,MPPTLow,MPPTHi,TambLow,TambHi,Weight
		0,0,0,0,0.0,0.0,0.0,0,0,0,0,0,0.0,0.0,0.0,0.0,0.0,0.0,0.0,0.0


		pacs = pvsystem.adrinverter(vdcs, pdcs, inverters[testinv])
		assert_series_equal(pacs, pd.Series([-0.25000, 105.635043, 503.769061]))

port pvl_adrinverter #288

port pvl_adrinverter #288

Conversation

VolkrB commented Jan 4, 2017

Need to document after everything looks good

wholmgren 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

VolkrB commented Jan 5, 2017

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 Jan 9, 2017

VolkrB commented Jan 9, 2017

adriesse commented Jan 9, 2017

VolkrB commented Jan 10, 2017

wholmgren 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

VolkrB commented Jan 17, 2017

wholmgren commented Jan 17, 2017

Choose a reason for hiding this comment

adriesse commented Jan 17, 2017

VolkrB commented Jan 17, 2017

VolkrB commented Jan 17, 2017

adriesse commented Jan 18, 2017

VolkrB commented Jan 19, 2017

adriesse Jan 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriesse commented Jan 19, 2017

adriesse commented Jan 19, 2017

jforbess commented Jan 20, 2017 via email

wholmgren commented Jan 26, 2017

adriesse commented Jan 28, 2017

wholmgren commented Jan 28, 2017

adriesse commented Jan 28, 2017

wholmgren commented Jan 28, 2017

wholmgren commented Feb 7, 2017

VolkrB commented Feb 7, 2017

wholmgren commented Feb 7, 2017

VolkrB commented Feb 7, 2017

wholmgren 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

VolkrB commented Feb 14, 2017

wholmgren commented Feb 14, 2017

VolkrB commented Feb 14, 2017

wholmgren commented Feb 14, 2017

VolkrB commented Feb 14, 2017

adriesse commented Feb 14, 2017

adriesse Jan 19, 2017 •

edited

Loading