Function to calculate temperature coefficient of power for pvsyst #1190

cwhanse · 2021-03-09T22:52:52Z

~~[ ] Closes #xxxx~~
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.

The temperature coefficient of power is not an input to the pvsyst single diode model. Rather, it is a result of the model, the derivative of power with respect to cell temperature, at the maximum power point, at reference conditions.

…efficient

…vsyst_tempco

cwhanse · 2021-03-15T03:20:55Z

Ready for review

wholmgren · 2021-03-16T15:32:26Z

pvlib/ivtools/sdm.py

 from pvlib.pvsystem import singlediode, v_from_i
+from pvlib.pvsystem import calcparams_pvsyst


Suggested change

from pvlib.pvsystem import singlediode, v_from_i

from pvlib.pvsystem import calcparams_pvsyst

from pvlib.pvsystem import calcparams_pvsyst, singlediode, v_from_i

wholmgren · 2021-03-16T15:37:14Z

pvlib/ivtools/sdm.py

+    -------
+    gamma_pmp : float
+        Temperature coefficient of power at maximum power point at reference
+        conditions. [%/C]


I was surprised to see %/C since the units of gamma_pdc are 1/C in pvwatts_dc. But I see we do use %/C in sdm.py. So either is fine with me at this point.

+1 to 1/C, more consistent with pvlib and the downstream PV Insight use. I probably cut/pasted from sdm.fit_cec_sam then didn't think about the interface again. We use %/C in sdm.fit_cec_sam because that's the quantity SAM requests.

wholmgren · 2021-03-16T15:38:13Z

pvlib/ivtools/sdm.py

+        Temperature coefficient of power at maximum power point at reference
+        conditions. [%/C]
+
+    p_mp_ref : float


A little strange to see gamma_pmp (p m p) and then p_mp_ref (p underscore m p)

Also a little strange to see pmp returned by a function that is ostensibly for finding temperature coefficient. I might argue that it would make more sense to either just return gamma_pmp or change the name of the function and also return vmp and imp. But I don't feel strongly about it.

Pvsyst doesn't name this parameter, so I'll call it gamma_pdc with units of 1/C, same as is used by the PVWatts model. If the function is returning a value with units of 1/C, then p_mp_ref isn't needed (it is if the unit is %/C).

wholmgren · 2021-03-16T15:48:32Z

pvlib/tests/ivtools/test_sdm.py

+    res_hiT = pvsystem.singlediode(*params_hiT)
+    expected = (res_hiT['p_mp'] - res_lowT['p_mp']) / 2.
+    # convert to %/C
+    expected = expected * 100 / (0.5 * (res_hiT['p_mp'] + res_lowT['p_mp']))


I'd prefer for this value to be hard coded and for the function calls above to be removed.

wholmgren · 2021-03-16T15:50:18Z

pvlib/tests/ivtools/test_sdm.py

+    expected = (res_hiT['p_mp'] - res_lowT['p_mp']) / 2.
+    # convert to %/C
+    expected = expected * 100 / (0.5 * (res_hiT['p_mp'] + res_lowT['p_mp']))
+    gamma_pdc, pdc_0 = sdm.pvsyst_temperature_coeff(**params)


I'm going to be a stickler again for passing args as args and kwargs as kwargs because it helps guard against unintended API changes.

or we can make them keyword-only arguments without defaults, though I don't think we've introduced that pattern in pvlib.

…vsyst_tempco

… into pvsyst_tempco

wholmgren · 2021-03-16T19:33:01Z

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

 * Added :py:func:`~pvlib.temperature.noct_sam`, a cell temperature model
+  implemented in SAM. (:pull:`1177`)
+* Added :py:func:`~pvlib.ivtools.sdm.pvsyst_temperature_coeff` to calculate
+  the temperature coefficient of power for the pvsyst module model.
+   (:pull:`1190`)
  implemented in SAM (:pull:`1177`, :pull:`1195`)


wholmgren · 2021-03-16T19:36:39Z

pvlib/tests/ivtools/test_sdm.py

+    params = {'alpha_sc': 0., 'gamma_ref': 1.1, 'mu_gamma': 0.,
+              'I_L_ref': 6., 'I_o_ref': 5.e-9, 'R_sh_ref': 200.,
+              'R_sh_0': 2000., 'R_s': 0.5, 'cells_in_series': 60}
+    expected = -0.7489980887568493


Seems a bit large for a realistic module. I don't have enough intuition with the params values to say more. I just want to double check that this really is the expected value and the function really is returning the right thing.

Good catch. The value was in W/C. Fixed now.

wholmgren

merging tomorrow morning unless there is further discussion.

wholmgren · 2021-03-16T21:15:50Z

pvlib/tests/ivtools/test_sdm.py

+    params = {'alpha_sc': 0., 'gamma_ref': 1.1, 'mu_gamma': 0.,
+              'I_L_ref': 6., 'I_o_ref': 5.e-9, 'R_sh_ref': 200.,
+              'R_sh_0': 2000., 'R_s': 0.5, 'cells_in_series': 60}
+    expected = -0.004886706494879083


Suggested change

expected = -0.004886706494879083

expected = -0.004887

In the past @adriesse has gently nudged us towards using reasonable precision when testing expected. While it can be a pain, I do think it makes the tests more understandable and makes it easier to catch issues like the one you fixed in the last commit.

I don't object on principle to the suggestion. But here I think more precision is useful, in case the test goes wonky in the future, and someone is reconstructing the expected value to know they have that calculation the same as when the test was created.

I only vaguely recall the nudging occasion, but I have a general pet peeve about people producing too many digits of noise (e.g. NSRDB). You can't even compress it!

For tests I think it depends on what the test is aiming to accomplish. In this case you might see what happens if someone passes in float32 or maybe even float16. I often use float32.

Looking in the code now it seems like lots of potential changes/improvements in the functions that are called could affect the numeric result without breaking the fundamental calculation. Do you want that detected?

One thing you could perhaps consider would be to not rely on defaults when calling derivative. Or do away with derivative and call pmax twice yourself. Then the user doesn't have to go to scipy documentation to figure out what is actually being done.

Good points about the options in scipy.misc.derivative. I didn't see an advantage to explicitly choosing options, or alternatively, hardcoding a particular derivative rule, but I'm open to doing either.

Peeve acknowledged and shared. The precision for expected is to enable reconstructing this value and being confident that the reconstruction is the same as the test originator's calculation.

I wasn't thinking about detecting small deviations in the output of pvsyst_temperature_coeff, only producing a value that is "good enough" in some sense. That's why I set rtol=0.0005; I think 4 digits agreement is enough to say two temperature coefficients are the same.

Sorry, I hadn't noticed the rtol. I got mislead by all those digit thinking the must be there because you wanted them to be there.

@cwhanse do you want to make revisions or go ahead and merge? I agree that @adriesse makes good points, but I am also fine with saying go read the scipy documentation if you want that level of detail.

I'm OK merging this and improving it later, if issues arise. The only options are point spacing and order of the derivative rule, and the defaults (dx=1.0C, order=3) are reasonable for this case.

wholmgren · 2021-03-18T17:59:43Z

thanks @cwhanse and @adriesse

cwhanse added 2 commits March 9, 2021 15:49

initial implementation of function to calculate pvsyst temperature co…

1e08d13

…efficient

add reference

a52a363

cwhanse mentioned this pull request Mar 12, 2021

Add compute functions for use case 2A SolarPerformanceInsight/solarperformanceinsight#169

Merged

cwhanse added 5 commits March 14, 2021 18:21

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

8bf93a0

…vsyst_tempco

add test for pvsyst_tempco

5b9a0ad

correct test reference

7002e56

api, whatsnews

f47f2da

add to whatsnew

c87fd14

cwhanse added this to the 0.9.0 milestone Mar 15, 2021

cwhanse added enhancement SPI DOE SETO Solar Performance Insight project labels Mar 15, 2021

alorenzo175 mentioned this pull request Mar 15, 2021

UC2A: Update _gamma for PVsyst modules SolarPerformanceInsight/solarperformanceinsight#174

Closed

wholmgren reviewed Mar 16, 2021

View reviewed changes

cwhanse added 5 commits March 16, 2021 12:26

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

d2177d8

…vsyst_tempco

change to 1/C, edit test

27e28cb

Merge branch 'master' into pvsyst_tempco

daeee87

merge upstream

73d68ba

Merge branch 'pvsyst_tempco' of https://github.com/cwhanse/pvlib-python…

2721784

… into pvsyst_tempco

wholmgren reviewed Mar 16, 2021

View reviewed changes

cwhanse added 2 commits March 16, 2021 13:54

fix bad merge

b6ccb17

change from W/C to 1/C

4cb885e

wholmgren approved these changes Mar 16, 2021

View reviewed changes

wholmgren merged commit 3269dc7 into pvlib:master Mar 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Function to calculate temperature coefficient of power for pvsyst #1190

Function to calculate temperature coefficient of power for pvsyst #1190

cwhanse commented Mar 9, 2021 •

edited

Loading

cwhanse commented Mar 15, 2021

wholmgren Mar 16, 2021

wholmgren Mar 16, 2021

cwhanse Mar 16, 2021

wholmgren Mar 16, 2021

wholmgren Mar 16, 2021

cwhanse Mar 16, 2021

wholmgren Mar 16, 2021

wholmgren Mar 16, 2021

wholmgren Mar 16, 2021

wholmgren Mar 16, 2021

wholmgren Mar 16, 2021

cwhanse Mar 16, 2021

wholmgren left a comment

wholmgren Mar 16, 2021

wholmgren Mar 16, 2021

cwhanse Mar 16, 2021 •

edited

Loading

adriesse Mar 17, 2021

cwhanse Mar 17, 2021

adriesse Mar 17, 2021

wholmgren Mar 18, 2021

cwhanse Mar 18, 2021

wholmgren commented Mar 18, 2021

		from pvlib.pvsystem import singlediode, v_from_i
		from pvlib.pvsystem import calcparams_pvsyst

Function to calculate temperature coefficient of power for pvsyst #1190

Function to calculate temperature coefficient of power for pvsyst #1190

Conversation

cwhanse commented Mar 9, 2021 • edited Loading

cwhanse commented Mar 15, 2021

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

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

cwhanse Mar 16, 2021 • edited Loading

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

wholmgren commented Mar 18, 2021

cwhanse commented Mar 9, 2021 •

edited

Loading

cwhanse Mar 16, 2021 •

edited

Loading