Repository: pvlib/pvlib-python
Pull request #651 (2019-01-22 20:22:59)

Improve clarity of code in pvsyst_celltemp (names and doc)
 - [x] Improves on the implementation of issue #552
 - [x] I am familiar with the [contributing guidelines](http://pvlib-python.readthedocs.io/en/latest/contributing.html).
 - [x] Fully tested. Added and/or modified tests to ensure correct behavior for all reasonable inputs. Tests (usually) must pass on the TravisCI and Appveyor testing services.
 - [x] Updates entries to `docs/sphinx/source/api.rst` for API changes.
 - [x] Adds description and name entries in the appropriate `docs/sphinx/source/whatsnew` file for all changes.
 - [x] Code quality and style is sufficient. Passes LGTM and SticklerCI checks.
 - [x] New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
 - [x] Pull request is nearly complete and ready for detailed review.

Brief description of the problem and proposed solution (if not already fully described in the issue linked to above):

I have tried to improve clarity of code in `pvsyst_celltemp` through changes to variables names and docstring, and also changed the name of one function parameter.



adriesse: Here are some additional thoughts about things I did not include in the PR:

- In the function argument list, I believe windspeed would be better placed *after* temperature, and given a default value--probably zero.
- If alpha is to be exposed, then it could/should be dealt with in the same way as the loss factors rather than as an independent function argument.  If this were done, then the name `loss_factors` would definitely no longer be appropriate, and I would suggest something like `model_parms` instead.
- For my own work I still like dictionaries for presets, so my functions don't need to know about them.  So I would use:
  - `pvsyst_celltemp (poa, wind, temp, **freestanding)` or 
  - `pvsyst_celltemp (poa, wind, temp, **model_parms['freestanding'])`

I do have some new work on temperature models which will raise some additional interesting code integration questions, but that's still a few months away at best.
wholmgren: > In the function argument list, I believe windspeed would be better placed after temperature, and given a default value--probably zero.

> For my own work I still like dictionaries for presets, so my functions don't need to know about them. So I would use: `pvsyst_celltemp (poa, wind, temp, **freestanding)` or `pvsyst_celltemp (poa, wind, temp, **model_parms['freestanding'])`

I agree with these proposals. They make the low level functions more flexible and more explicit. The doc strings can point users to the presets in the module-level dictionary. The `PVSystem` and `ModelChain` abstractions can still make the function a bit easier to use for common use cases.

We'd need a deprecation cycle to make the same changes to `sapm_celltemp`. I don't have a  preference about if that happens for 0.6.1 or not. 
adriesse: Further changes anyone?
wholmgren: Ok to merge if CI passes.

Patch:
diff --git a/.codecov.yml b/.codecov.yml
index f8a38af..48df15e 100644
--- a/.codecov.yml
+++ b/.codecov.yml
@@ -20,8 +20,8 @@ coverage:
         paths:
           - "pvlib/(\w+/)?[^/]+\.py$"
       tests:
-        target: 96%
+        target: 95%
         paths:
           - "pvlib/test/.*"
 
-comment: off
\ No newline at end of file
+comment: off
diff --git a/docs/sphinx/source/whatsnew/v0.6.1.rst b/docs/sphinx/source/whatsnew/v0.6.1.rst
index 6721b0a..d4f132b 100644
--- a/docs/sphinx/source/whatsnew/v0.6.1.rst
+++ b/docs/sphinx/source/whatsnew/v0.6.1.rst
@@ -101,3 +101,4 @@ Contributors
 * Cameron Stark (:ghuser:`camerontstark`)
 * Jonathan Gaffiot (:ghuser:`jgaffiot`)
 * Marc Anoma (:ghuser:`anomam`)
+* Anton Driesse (:ghuser:`adriesse`)
diff --git a/pvlib/pvsystem.py b/pvlib/pvsystem.py
index 0dfdb07..35e4833 100644
--- a/pvlib/pvsystem.py
+++ b/pvlib/pvsystem.py
@@ -46,7 +46,7 @@ DC_MODEL_PARAMS = {
 }
 
 
-TEMP_MODELS = {
+TEMP_MODEL_PARAMS = {
     'sapm': {'open_rack_cell_glassback': (-3.47, -.0594, 3),
              'roof_mount_cell_glassback': (-2.98, -.0471, 1),
              'open_rack_cell_polymerback': (-3.56, -.0750, 3),
@@ -521,7 +521,7 @@ class PVSystem(object):
             poa_direct, poa_diffuse, airmass_absolute, aoi,
             self.module_parameters, reference_irradiance=reference_irradiance)
 
-    def pvsyst_celltemp(self, poa_global, wind_speed, temp_air):
+    def pvsyst_celltemp(self, poa_global, temp_air, wind_speed=1.0):
         """Uses :py:func:`pvsyst_celltemp` to calculate module temperatures
         based on ``self.racking_model`` and the input parameters.
 
@@ -535,8 +535,8 @@ class PVSystem(object):
         """
         kwargs = _build_kwargs(['eta_m', 'alpha_absorption'],
                                self.module_parameters)
-        return pvsyst_celltemp(poa_global, wind_speed, temp_air,
-                               temp_model=self.racking_model, **kwargs)
+        return pvsyst_celltemp(poa_global, temp_air, wind_speed,
+                               model_params=self.racking_model, **kwargs)
 
     def first_solar_spectral_loss(self, pw, airmass_absolute):
 
@@ -1887,7 +1887,7 @@ def sapm_celltemp(poa_global, wind_speed, temp_air,
     sapm
     '''
 
-    temp_models = TEMP_MODELS['sapm']
+    temp_models = TEMP_MODEL_PARAMS['sapm']
 
     if isinstance(model, str):
         model = temp_models[model.lower()]
@@ -1908,30 +1908,37 @@ def sapm_celltemp(poa_global, wind_speed, temp_air,
     return pd.DataFrame({'temp_cell': temp_cell, 'temp_module': temp_module})
 
 
-def pvsyst_celltemp(poa_global, wind_speed, temp_air, eta_m=0.1,
-                    alpha_absorption=0.9, temp_model="freestanding"):
+def pvsyst_celltemp(poa_global, temp_air, wind_speed=1.0, eta_m=0.1,
+                    alpha_absorption=0.9, model_params='freestanding'):
     """
-    Calculate cell temperature using the PVSyst model.
+    Calculate cell temperature using an emperical heat loss factor model
+    as implemented in PVsyst.
+
+    The heat loss factors provided through the 'model_params' argument
+    represent the combined effect of convection, radiation and conduction,
+    and their values are experimentally determined.
 
     Parameters
     ----------
     poa_global : numeric
         Total incident irradiance in W/m^2.
 
-    wind_speed : numeric
-        Wind speed in m/s at a height of 10 meters.
-
     temp_air : numeric
         Ambient dry bulb temperature in degrees C.
 
-    eta_m : numeric
+    wind_speed : numeric, default 1.0
+        Wind speed in m/s measured at the same height for which the wind loss
+        factor was determined.  The default value is 1.0, which is the wind
+        speed at module height used to determine NOCT.
+
+    eta_m : numeric, default 0.1
         Module external efficiency as a fraction, i.e., DC power / poa_global.
 
-    alpha_absorption : float
-        Absorption coefficient, default is 0.9.
+    alpha_absorption : numeric, default 0.9
+        Absorption coefficient
 
-    temp_model : string, tuple, or list, default 'freestanding' (no dict)
-        Model to be used.
+    model_params : string, tuple, or list (no dict), default 'freestanding'
+        Heat loss factors to be used.
 
         If string, can be:
 
@@ -1944,12 +1951,12 @@ def pvsyst_celltemp(poa_global, wind_speed, temp_air, eta_m=0.1,
 
         If tuple/list, supply parameters in the following order:
 
-            * natural_convenction_coeff : float
-                Natural convection coefficient. Freestanding default is 29,
-                fully insulated arrays is 15.
+            * constant_loss_factor : float
+                Combined heat loss factor coefficient. Freestanding
+                default is 29, fully insulated arrays is 15.
 
-            * forced_convection_coeff : float
-                Forced convection coefficient, default is 0.
+            * wind_loss_factor : float
+                Combined heat loss factor influenced by wind. Default is 0.
 
     Returns
     -------
@@ -1965,25 +1972,21 @@ def pvsyst_celltemp(poa_global, wind_speed, temp_air, eta_m=0.1,
     photovoltaic modules." Progress in Photovoltaics 16(4): 307-315.
     """
 
-    temp_models = TEMP_MODELS['pvsyst']
+    pvsyst_presets = TEMP_MODEL_PARAMS['pvsyst']
 
-    if isinstance(temp_model, str):
-        natural_convenction_coeff, forced_convection_coeff = temp_models[
-            temp_model.lower()
-        ]
-    elif isinstance(temp_model, (tuple, list)):
-        natural_convenction_coeff, forced_convection_coeff = temp_model
+    if isinstance(model_params, str):
+        model_params = model_params.lower()
+        constant_loss_factor, wind_loss_factor = pvsyst_presets[model_params]
+    elif isinstance(model_params, (tuple, list)):
+        constant_loss_factor, wind_loss_factor = model_params
     else:
         raise TypeError(
-            "Please format temp_model as a str, or tuple/list."
+            "Please provide model_params as a str, or tuple/list."
         )
 
-    combined_convection_coeff = (
-        forced_convection_coeff * wind_speed
-    ) + natural_convenction_coeff
-
-    absorption_coeff = alpha_absorption * poa_global * (1 - eta_m)
-    temp_difference = absorption_coeff / combined_convection_coeff
+    total_loss_factor = wind_loss_factor * wind_speed + constant_loss_factor
+    heat_input = poa_global * alpha_absorption * (1 - eta_m)
+    temp_difference = heat_input / total_loss_factor
     temp_cell = temp_air + temp_difference
 
     return temp_cell


Test patch:
diff --git a/pvlib/test/test_pvsystem.py b/pvlib/test/test_pvsystem.py
index 4a1ef52..2f0eeb8 100644
--- a/pvlib/test/test_pvsystem.py
+++ b/pvlib/test/test_pvsystem.py
@@ -1079,32 +1079,32 @@ def test_PVSystem_sapm_celltemp(mocker):
 
 
 def test_pvsyst_celltemp_default():
-    default = pvsystem.pvsyst_celltemp(900, 5, 20, 0.1)
+    default = pvsystem.pvsyst_celltemp(900, 20, 5)
     assert_allclose(default, 45.137, 0.001)
 
 
 def test_pvsyst_celltemp_non_model():
-    tup_non_model = pvsystem.pvsyst_celltemp(900, 5, 20, 0.1,
-                                             temp_model=(23.5, 6.25))
+    tup_non_model = pvsystem.pvsyst_celltemp(900, 20, 5, 0.1,
+                                             model_params=(23.5, 6.25))
     assert_allclose(tup_non_model, 33.315, 0.001)
 
-    list_non_model = pvsystem.pvsyst_celltemp(900, 5, 20, 0.1,
-                                              temp_model=[26.5, 7.68])
+    list_non_model = pvsystem.pvsyst_celltemp(900, 20, 5, 0.1,
+                                              model_params=[26.5, 7.68])
     assert_allclose(list_non_model, 31.233, 0.001)
 
 
 def test_pvsyst_celltemp_model_wrong_type():
     with pytest.raises(TypeError):
         pvsystem.pvsyst_celltemp(
-            900, 5, 20, 0.1,
-            temp_model={"won't": 23.5, "work": 7.68})
+            900, 20, 5, 0.1,
+            model_params={"won't": 23.5, "work": 7.68})
 
 
 def test_pvsyst_celltemp_model_non_option():
     with pytest.raises(KeyError):
         pvsystem.pvsyst_celltemp(
-            900, 5, 20, 0.1,
-            temp_model="not_an_option")
+            900, 20, 5, 0.1,
+            model_params="not_an_option")
 
 
 def test_pvsyst_celltemp_with_index():
@@ -1113,7 +1113,7 @@ def test_pvsyst_celltemp_with_index():
     irrads = pd.Series([0, 500, 0], index=times)
     winds = pd.Series([10, 5, 0], index=times)
 
-    pvtemps = pvsystem.pvsyst_celltemp(irrads, winds, temps)
+    pvtemps = pvsystem.pvsyst_celltemp(irrads, temps, wind_speed=winds)
     expected = pd.Series([0.0, 23.96551, 5.0], index=times)
     assert_series_equal(expected, pvtemps)
 
@@ -1131,9 +1131,9 @@ def test_PVSystem_pvsyst_celltemp(mocker):
     irrad = 800
     temp = 45
     wind = 0.5
-    out = system.pvsyst_celltemp(irrad, wind, temp)
+    out = system.pvsyst_celltemp(irrad, temp, wind_speed=wind)
     pvsystem.pvsyst_celltemp.assert_called_once_with(
-        irrad, wind, temp, eta_m, alpha_absorption, racking_model)
+        irrad, temp, wind, eta_m, alpha_absorption, racking_model)
     assert isinstance(out, float)
     assert out < 90 and out > 70