Implement calcparams_pvsyst and tests #486

cwhanse · 2018-06-20T14:37:40Z

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 Implement PVsyst single diode model #470
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):

Implement calcparams_pvsyst for use with singlediode

wholmgren

Looks good to me except for a few doc string issues. I don't have any experience with the pvsyst model, so I'll leave it to others to comment on the details.

wholmgren · 2018-06-20T16:28:19Z

pvlib/pvsystem.py

+
+    gamma_ref : float
+        The diode ideality factor
+


missing mugamma documentation here. mugamma or mu_gamma?

added. changed to mu_gamma

it appears that the change was not applied here.

wholmgren · 2018-06-20T16:31:46Z

pvlib/pvsystem.py

+            The average cell temperature of cells within a module in C.
+
+        **kwargs
+            See pvsystem.calcparams_desoto for details


pvsyst, not desoto.

cwhanse · 2018-06-20T21:09:29Z

@mikofski would appreciate your review of calcparams_pvsyst

adriesse

We will certainly exercise this code during the next few months!

adriesse · 2018-06-20T21:46:09Z

pvlib/pvsystem.py

+    photocurrent : numeric
+        Light-generated current in amperes at irradiance=S and
+        cell temperature=Tcell.
+


I edited the output description (and in pvsystem.calcparams_desoto) to remove the phrase 'at irradiance....=Tcell' I think the context is clear.

adriesse · 2018-06-20T21:46:27Z

pvlib/pvsystem.py

+    saturation_current : numeric
+        Diode saturation curent in amperes at irradiance
+        S and cell temperature Tcell.
+


adriesse · 2018-06-20T21:46:48Z

pvlib/pvsystem.py

+    resistance_series : float
+        Series resistance in ohms at irradiance S and cell temperature
+        Tcell.
+


adriesse · 2018-06-20T21:47:01Z

pvlib/pvsystem.py

+    resistance_shunt : numeric
+        Shunt resistance in ohms at irradiance S and cell temperature
+        Tcell.
+


adriesse · 2018-06-20T21:48:47Z

pvlib/pvsystem.py

+    # reference temperature
+    Tref_K = temp_ref + 273.15
+    Tcell_K = temp_cell + 273.15
+


temp_ref_K ?
temp_cell_K ?

these are local variables, so I think its OK that they are different in style.

So I changed them as you suggested :)

markcampanelli · 2018-06-21T00:04:01Z

pvlib/pvsystem.py

+    R_s : float
+        The series resistance at reference conditions, in ohms.
+
+    cells_in_series : integer


Ns would seem more consistent with the desoto model and the nNsVth variable.

I agree but we use cells_in_series as the key for the SAPM parameters, and some have a preference for descriptive variable names.

markcampanelli · 2018-06-21T00:06:02Z

pvlib/pvsystem.py

+    R_sh_0 : float
+        The shunt resistance at zero irradiance conditions, in ohms.
+
+    R_s : float


R_s_ref seems more consistent with other variables at reference conditions.

I agree. But here we run into an unfortunate linkage to the SAM database for the CEC model parameters. R_s is the heading in that file, which is used to define the keys for the module parameter dict. Although in my opinion R_s for PVsyst is a different parameter than R_s for De Soto or CEC (because parameter values are determined jointly, they are intrinsically linked and can't be regarded as correct for a different model) I do think it's useful to maintain a common nomenclature.

I think changing R_s to R_s_ref will involve some work in pvsystem.retrieve_sam so I'm not going to take that on in this PR.

markcampanelli · 2018-06-21T00:08:23Z

pvlib/pvsystem.py

+    gamma_ref : float
+        The diode ideality factor
+
+    mugamma : float


Perhaps mu_gamma?

markcampanelli · 2018-06-21T00:09:46Z

pvlib/pvsystem.py

+    Tuple of the following results:
+
+    photocurrent : numeric
+        Light-generated current in amperes at irradiance=S and


Not sure about irradiance=S here and below.

markcampanelli · 2018-06-21T00:13:16Z

@cwhanse Are you not planning to integrate the pvsyst model into ModelChain in this PR?

markcampanelli · 2018-06-21T04:36:09Z

@mikofski This was by no means a criticism. Rather, I was just wondering if complete integration was expected right away. LIkewise, #478 is much more easily implemented if I can worry about the ModelChain integration as a followup step.

@cwhanse I do propose this PR adds a usage example in a subsection of docs/tutorials/pvsystem.ipynb.

mikofski · 2018-06-21T05:48:37Z

I'm so sorry @thunderfish24 my comment was snarky. It was a fair question.

markcampanelli · 2018-06-21T06:40:09Z

@mikofski No worries mate! My question could have been phrased better, and it's often easy to misinterpret typings when we are not face-to-face.

cwhanse · 2018-06-21T13:30:39Z

@mikofski @thunderfish24 yes I'll open a new Issue and PR for extending ModelChain for new single diode models.

wholmgren

A couple more minor things but otherwise looks good to me.

wholmgren · 2018-06-23T20:45:06Z

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

@@ -13,6 +13,7 @@ Enhancements
 * Add sea surface albedo in irradiance.py (:issue:`458`)
 * Implement first_solar_spectral_loss in modelchain.py (:issue:`359`)
 * Clarify arguments Egref and dEgdT for calcparams_desoto (:issue:`462`)
+* Add pvsystem.calcparams_pvsyst to compute values for the single diode equation using the PVsyst v6 model.


(:issue:470)

wholmgren · 2018-06-23T20:48:22Z

pvlib/pvsystem.py

+
+    gamma_ref : float
+        The diode ideality factor
+


it appears that the change was not applied here.

wholmgren

Can you look into setting your python file editor/IDE to strip extra white space from the end of lines in your new code? This makes diffs much easier to understand in the long run.

wholmgren · 2018-06-24T00:51:15Z

pvlib/test/test_pvsystem.py

+def pvsyst_module_params():
+    module_parameters = {}
+    module_parameters['gamma_ref'] = 1.05
+    module_parameters['mugamma'] = 0.001


test fixture and the two tests below need mugamma --> mu_gamma

cwhanse · 2018-06-27T16:35:44Z

I changed a setting in Spyder, hopefully that improves the white space issue. Test failures are for read_tmy3

wholmgren · 2018-07-04T17:25:58Z

pvlib/pvsystem.py

@@ -307,6 +307,36 @@ def calcparams_desoto(self, effective_irradiance, temp_cell, **kwargs):

        return calcparams_desoto(effective_irradiance, temp_cell, **kwargs)

+    def calcparams_pvsyst(self, effective_irradiance, temp_cell, **kwargs):


similar to what was pointed out by @thunderfish24 in #478... the **kwargs in this method signature don't do anything, so we should remove **kwargs from the method signature. To be clear, we need to keep them in the function call below.

wholmgren · 2018-07-04T17:27:21Z

pvlib/pvsystem.py

+        kwargs = _build_kwargs(['gamma_ref', 'mu_gamma', 'I_L_ref', 'I_o_ref', 
+                                'R_sh_ref', 'R_sh_0', 'R_sh_exp', 
+                                'R_s', 'alpha_sc', 'EgRef',
+                                'cells_in_series'],


should 'irrad_ref' and 'temp_ref' be added to this lookup list? I'd say yes because they seem intrinsic to the module specification.

done. I also added these keywords to calcparams_desoto

wholmgren · 2018-07-04T17:28:23Z

pvlib/pvsystem.py

+            The average cell temperature of cells within a module in C.
+
+        **kwargs
+            See pvsystem.calcparams_pvsyst for details


and delete **kwargs from the docstring.

wholmgren · 2018-07-04T17:29:49Z

I think this should be merged after resolving the minor issues around **kwargs

…f and temp_ref keyword

wholmgren · 2018-07-05T22:24:56Z

@cwhanse ok with me if you go ahead and "squash and merge" and then delete the branch from the organization's repository. It should give you a delete option right here once the branch is merged.

markcampanelli

Sorry for my late review. I did have one question about the model that I'd like clarification on. Everything else is small potatoes.

markcampanelli · 2018-07-06T04:57:24Z

pvlib/pvsystem.py

+    model described in [1,2,3]. The five values returned by calcparams_pvsyst
+    can be used by singlediode to calculate an IV curve.
+
+    Parameters


What is the difference between numeric and float?

Should a user NOT expect this function to be vectorized over the "float" parameters?

I'm pretty sure the function will work on array or Series inputs. Is the issue the type description float implies a singleton?

We aren't using numeric or float consistently in the comments. For me to fix that, requires that I learn the distinction. New issue, I think.

I've used numeric for input that can be scalar, np.array, or pd.Series. float is specific to scalar. Whether or not this is a good idea could be discussed in a separate issue, if necessary.

for more context, note that some functions require "array-like" (np.array or pd.Series) and others require pd.Series.

So, numeric unless there's a reason to specify a scalar float?

or unless there's a reason to specify array-like or Series.

I looked at both calcparams_desoto and calcparams_pvsyst and I'm going to leave the docstrings as is for now. The typical use of either function is to pass model parameters for one module and a list of conditions (irradiance and temperature). That use is reflected in the docstring's use of numeric and float, where each model parameter is a single value for a module. The functions are more flexible that this use case.

If we want to overhaul the docstrings, let's open a new issue for it.

I think an issue is wise in order to define what we mean to ourselves/users and make everything consistent.

markcampanelli · 2018-07-06T04:59:16Z

pvlib/pvsystem.py

+    saturation_current : numeric
+        Diode saturation current in amperes
+
+    resistance_series : float


Seems like this should be "numeric" too.

markcampanelli · 2018-07-06T05:00:45Z

pvlib/pvsystem.py


    resistance_series : float
-        Series resistance in ohms at irradiance S and cell temperature
-        Tcell.
+        Series resistance in ohms


While passing through this territory, seems like this should be changed to "numeric" from "float".

Cut and paste inheritance? I don't think there is a difference here. numeric would admit long, complex and int types as well as float. Probably should change to float unless there's a reason otherwise.

I'm good with float instead of numeric, because that seems to be most accurate.

markcampanelli · 2018-07-06T05:04:06Z

pvlib/pvsystem.py

+                                'cells_in_series'],
+                                self.module_parameters)
+
+        return calcparams_pvsyst(effective_irradiance, temp_cell, **kwargs)


This could be done in a separate issue (see #476), but I think the PVSystem methods should scale the outputs to the modules_per_string and strings_per_inverter.

We've chosen not to scale inside dc_model functions to offer flexibility, e.g., model a system with strings of different lengths, to implement module-to-module mismatch models, etc. Granted some of these capabilities are not presently in pvlib.

markcampanelli · 2018-07-06T05:12:05Z

pvlib/pvsystem.py

+    '''
+
+    # Boltzmann constant in J/K
+    k = 1.3806488e-23


Appears to differ a bit from scipy.constants.Boltzmann, which is 1.38064852e-23

Good catch. That's a typo.

markcampanelli · 2018-07-06T05:35:40Z

pvlib/pvsystem.py

+          (np.exp((q * EgRef) / (k * gamma) * (1 / Tref_K - 1 / Tcell_K)))
+
+    Rsh_tmp = (R_sh_ref - R_sh_0 * np.exp(-R_sh_exp)) / (1.0 - np.exp(-R_sh_exp))
+    Rsh_base = np.maximum(0.0, Rsh_tmp)


In looking over http://files.pvsyst.com/help/pvmodule_rshexp.htm, it is not clear to me why Rsh_base is not simply R_sh_ref. What am I missing?

You're not, PVsyst documentation is missing that definition of Rsh_base. Ken Sauer and Thomas Roessler are my sources, they got it verbally from PVsyst.

markcampanelli · 2018-07-06T05:36:52Z

pvlib/pvsystem.py

+    nNsVth = gamma * k / q * cells_in_series * Tcell_K
+
+    IL = effective_irradiance / irrad_ref * \
+              (I_L_ref + alpha_sc * (Tcell_K - Tref_K))


Optimization possibility: Tcell_K - Tref_K is computed twice.

markcampanelli · 2018-07-06T05:37:20Z

pvlib/pvsystem.py

+    Rsh_base = np.maximum(0.0, Rsh_tmp)
+
+    Rsh = Rsh_base + (R_sh_0 - Rsh_base) * \
+              np.exp(-R_sh_exp * effective_irradiance / irrad_ref)


Optimization possibility: effective_irradiance / irrad_ref is computed twice.

markcampanelli · 2018-07-06T05:42:47Z

pvlib/test/test_pvsystem.py

+    assert_series_equal(np.round(Rsh, 3),
+                        pd.Series([1000.0, 305.757], index=times))
+    assert_series_equal(np.round(nNsVth, 4),
+                        pd.Series([1.6186, 1.7961], index=times))


My preference is to compute the expected values directly in the test. I'm curious if you have particular reasons not to do that?

I don't other than it was easy (for me) to compute the values in MATLAB and hardcode them here. My way is certainly less transparent.

I'm confused. I don't understand how general test values can be computed inside a test without duplicating the function itself.

Some view including a separate (re)computation as redundant, others view it as transparent. In this case I don't think any of the computations require an implicit solver, so where the values come from should not leave too much to question.

markcampanelli · 2018-07-06T05:51:08Z

pvlib/test/test_pvsystem.py

@@ -386,6 +403,35 @@ def test_calcparams_desoto(cec_module_params):
    assert_allclose(nNsVth, 0.473)


+def test_calcparams_pvsyst(pvsyst_module_params):


Not critical, but you might consider a simple "API" test that passes the five computed coefficients into singlediode just to make sure the computation runs. This makes sure that pvsyst gets updated if the API changes.

Fair point. I'm not going to put a new test in here because I don't clearly see how to separate issues being tested, and to avoid redundant tests. The current test_calcparams_pvsyst verifies correct output of pvsystem.calcparams_pvsyst. Adding a call to singlediode within test_calcparams_pvsyst adds a new purpose (API check) and I prefer to keep tests to a single purpose. Adding a new test that calls calcparams_pvsyst and then singlediode would duplicate the test of PVSystem.calcparams_pvsyst method.

Do you have an example in the campanelli pull request?

No, I haven't written any tests yet and I admit that I am not familiar with this particular mocker setup. If a mock call can verify the API, then that would be great. I don't mean to hold up this PR and I'll look into it further with the Campanelli model.

wholmgren · 2018-07-06T15:04:46Z

pvlib/test/test_pvsystem.py

@@ -174,6 +174,23 @@ def cec_module_params(sam_data):
    return module_parameters


+@pytest.fixture(scope="session")


not a big deal, but if we're still making changes this should not have scope="session"

I copied that. scope="module"?

no scope argument. @pytest.fixture and nothing else

The reasoning for this is that this function returns a mutable dictionary. In principle, one test could inadvertently modify the dictionary, and a second test could pass or fail because of that modification. @pytest.fixture will call the function and generate a new dictionary each time the data is needed. Adding scope="module" or scope="session" instructs pytest to recreate the data only once per test module (the fixture could be imported into a new module and data would be regenerated) or once per test session (this defeats a large part of the purpose of fixtures).

OK. I'll remove this instance of scope=session from the pvsyst module parameter fixture. Thanks for the explanation.

cwhanse · 2018-07-06T17:15:45Z

Looks like errors related to pulling tmy3 data from NREL's site.

markcampanelli

LGTM

wholmgren · 2018-07-06T19:21:35Z

@thunderfish24 thanks for your careful review -- it's a big help.

@cwhanse ready to merge?

cwhanse · 2018-07-09T19:00:46Z

Yes, ready to merge.

mikofski · 2018-08-03T22:22:40Z

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

@@ -13,6 +13,7 @@ Enhancements
 * Add sea surface albedo in irradiance.py (:issue:`458`)
 * Implement first_solar_spectral_loss in modelchain.py (:issue:`359`)
 * Clarify arguments Egref and dEgdT for calcparams_desoto (:issue:`462`)
+* Add pvsystem.calcparams_pvsyst to compute values for the single diode equation using the PVsyst v6 model (:issue:'470')


please change single spaces around issue #470 to backticks, and also surround pvsystem.calcparams_pvsyst with backticks - @wholmgren or @cwhanse , can you do this directly in master, thx

Add `pvsystem.calcparams_pvsyst` to compute values for the single diode equation using the PVsyst v6 model (:issue:`470`)

Cliff Hansen added 3 commits June 20, 2018 08:32

initial version of calcparams_pvsyst and test

e20a079

fix argument list in tests

e93cd8a

Fix typo in test argument R_sh_0

846c913

wholmgren reviewed Jun 20, 2018

View reviewed changes

Cliff Hansen added 7 commits June 20, 2018 10:41

remove EgRef from PVSystem.test_calcparams_pvsyst

f457370

add documentation for mugamma, fix typo

691a9a7

edits and fix to test error

36d5941

Fix argument list and expected result

131ff75

Fix error in calculation of nNsVth

9d5d55e

Add note to whatsnew.rst

d1be467

Add line to api.rst

8d79f3d

cwhanse changed the title ~~initial version of calcparams_pvsyst and test~~ Implement calcparams_pvsyst and tests Jun 20, 2018

adriesse reviewed Jun 20, 2018

View reviewed changes

markcampanelli reviewed Jun 21, 2018

View reviewed changes

Edits to function signatures, variable names

34390a9

wholmgren reviewed Jun 23, 2018

View reviewed changes

Change mugamma to mu_gamma, add issue to whatsnew text

95bfde9

wholmgren reviewed Jun 24, 2018

View reviewed changes

Change to mu_gamma in tests

75e89f0

wholmgren approved these changes Jun 27, 2018

View reviewed changes

wholmgren reviewed Jul 4, 2018

View reviewed changes

remove **kwargs from method calcparams_pvsyst signature, add irrad_re…

046a386

…f and temp_ref keyword

wholmgren added this to the 0.6.0 milestone Jul 5, 2018

wholmgren added the enhancement label Jul 5, 2018

wholmgren approved these changes Jul 5, 2018

View reviewed changes

markcampanelli reviewed Jul 6, 2018

View reviewed changes

fix typo in Boltzmann constant J/K

632ff87

wholmgren reviewed Jul 6, 2018

View reviewed changes

Remove scope=session from pvsyst fixture

1295bcf

markcampanelli approved these changes Jul 6, 2018

View reviewed changes

wholmgren merged commit ee03ed4 into master Jul 9, 2018

wholmgren deleted the calcparams_pvsyst branch July 9, 2018 20:35

mikofski reviewed Aug 3, 2018

View reviewed changes

		@@ -307,6 +307,36 @@ def calcparams_desoto(self, effective_irradiance, temp_cell, **kwargs):

		return calcparams_desoto(effective_irradiance, temp_cell, **kwargs)

		def calcparams_pvsyst(self, effective_irradiance, temp_cell, **kwargs):

		@@ -386,6 +403,35 @@ def test_calcparams_desoto(cec_module_params):
		assert_allclose(nNsVth, 0.473)


		def test_calcparams_pvsyst(pvsyst_module_params):

		@@ -174,6 +174,23 @@ def cec_module_params(sam_data):
		return module_parameters


		@pytest.fixture(scope="session")

Implement calcparams_pvsyst and tests #486

Implement calcparams_pvsyst and tests #486

Conversation

cwhanse commented Jun 20, 2018 • edited by wholmgren Loading

pvlib python pull request guidelines

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

cwhanse commented Jun 20, 2018

adriesse 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markcampanelli commented Jun 21, 2018

markcampanelli commented Jun 21, 2018

mikofski commented Jun 21, 2018

markcampanelli commented Jun 21, 2018

cwhanse commented Jun 21, 2018

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

wholmgren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwhanse commented Jun 27, 2018

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 Jul 4, 2018

wholmgren commented Jul 5, 2018 • edited Loading

markcampanelli 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

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

cwhanse commented Jun 20, 2018 •

edited by wholmgren

Loading

wholmgren commented Jul 5, 2018 •

edited

Loading

wholmgren Jul 6, 2018 •

edited

Loading