Add Array.get_cell_temperature, PVSystem.get_cell_temperature #1211

kandersolar · 2021-04-15T02:37:58Z

~~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.

Trying out the idea from #1176 (comment). If merged, I'd then update #1176 to have Array.get_cell_temperature grab the model parameters from the Array's Mount.

…recate old PVSystem tcell methods, update ModelChain._set_celltemp

kandersolar · 2021-04-15T02:51:57Z

PSM3 test failures presumably related to NREL/developer.nrel.gov#208

wholmgren

Overall this looks really good to me. The dancing around effective_irradiance is a little awkward, but I don't have a good recommendation for an alternative.

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

cwhanse · 2021-04-26T17:16:56Z

pvlib/modelchain.py

@@ -1009,26 +1009,26 @@ def _set_celltemp(self, model):
        temp_air = _tuple_from_dfs(self.results.weather, 'temp_air')
        wind_speed = _tuple_from_dfs(self.results.weather, 'wind_speed')
        kwargs = {}
-        if model == self.system.noct_sam_celltemp:
+        if model == 'noct_sam':


The docstring says that model is a function, it's changed to be a string here and elsewhere.

With this change, is ModelChain.temperature_model needed?

With this change, is ModelChain.temperature_model needed?

I think this question remains, but it should be explored in a separate issue.

pvlib/pvsystem.py

cwhanse · 2021-04-26T17:26:45Z

pvlib/pvsystem.py

+            params = _build_kwargs(['transmittance_absorptance', 'eta_m_ref',
+                                    'noct', 'array_height', 'mount_standoff'],
+                                   self.temperature_model_parameters)
+            if not {'noct', 'eta_m_ref'}.issubset(params):


Would be a good idea to parallel this error message for the other temperature models.

I'm not sure how I feel about this. Without a check like this, the error message isn't as helpful as it could be -- TypeError: noct_sam() missing 1 required positional argument: 'noct' makes no mention of temperature_model_parameters. But I'm not really a fan of having all these lists of pvlib.temperature parameter names in pvlib.pvsystem.

What do you think about letting python raise the TypeError and we add a hint to the error message like this:

try: temperature_cell = func(poa_global, temp_air, wind_speed, **params) except TypeError as e: msg = e.args[0] + ( ". Check temperature_model_parameters to make sure " "all required parameters are specified." ) raise TypeError(msg)

Downside is that it adds irrelevant noise to TypeErrors caused by other problems.

I won't stand in the way of this PR over error messaging. If we want to verify parameters at this stage, as a kindness to users, maybe a dict in pvlib.temperature akin to pvlib.pvsystem._DC_MODEL_PARAMS.

I think the problem is that we're using _build_kwargs to build all of the arguments instead of just the optional keyword arguments. I suggest a pattern that performs explicit look ups instead of deferred checks. Fine with me to use the standard python KeyError or break into the kinder message template.

args = (self.temperature_model_par[arg] for arg in required_args) kwargs = _build_kwargs(optional_args, self.temperature_model_parameters)

required_args and optional_args could come from a private dict like @cwhanse suggests. I'm also ok with hard coding it here (sounds easy). And I'm ok with using inspect (I vaguely recall considering this previously and thinking it was too complicated for pvlib, but I currently like it).

I added something along these lines, let me know what you think.

pvlib/pvsystem.py

wholmgren · 2021-04-27T15:46:23Z

pvlib/pvsystem.py

+            params = _build_kwargs(['transmittance_absorptance', 'eta_m_ref',
+                                    'noct', 'array_height', 'mount_standoff'],
+                                   self.temperature_model_parameters)
+            if not {'noct', 'eta_m_ref'}.issubset(params):


I think the problem is that we're using _build_kwargs to build all of the arguments instead of just the optional keyword arguments. I suggest a pattern that performs explicit look ups instead of deferred checks. Fine with me to use the standard python KeyError or break into the kinder message template.

args = (self.temperature_model_par[arg] for arg in required_args) kwargs = _build_kwargs(optional_args, self.temperature_model_parameters)

required_args and optional_args could come from a private dict like @cwhanse suggests. I'm also ok with hard coding it here (sounds easy). And I'm ok with using inspect (I vaguely recall considering this previously and thinking it was too complicated for pvlib, but I currently like it).

wholmgren · 2021-04-27T15:46:54Z

pvlib/pvsystem.py

+
+        # allow kwargs to override
+        params.update(kwargs)
+        temperature_cell = func(poa_global, temp_air, wind_speed, **params)


and here I would use *args and **kwargs

Co-authored-by: Cliff Hansen <[email protected]>

pvlib/modelchain.py

pvlib/pvsystem.py

Co-authored-by: Cliff Hansen <[email protected]>

…python into array_tcell

kandersolar · 2021-05-14T01:16:39Z

Checks are green. Anything left on this one?

wholmgren

Close. A few minor things and a larger question about if we can replace **kwargs with effective_irradiance=None.

docs/sphinx/source/api.rst

pvlib/pvsystem.py

wholmgren · 2021-05-14T03:27:43Z

pvlib/tests/test_modelchain.py

    mc.run_model(weather)
    assert m_sapm.call_count == 1
    # assert_called_once_with cannot be used with series, so need to use
    # assert_series_equal on call_args
    assert_series_equal(m_sapm.call_args[0][1], weather['temp_air'])  # temp
    assert_series_equal(m_sapm.call_args[0][2], weather['wind_speed'])  # wind
+    assert m_sapm.call_args[1]['model'] == 'sapm'


No objection to this. An alternative is modifying the spy line to mocker.spy(temperature, 'sapm_cell') and dropping this line.

wholmgren · 2021-05-14T03:41:05Z

pvlib/pvsystem.py

+        # additional model-specific (and array-specific) inputs
+        extra_inputs = [{}] * self.num_arrays
+
+        if 'effective_irradiance' in kwargs:


Is there a reason we cannot add effective_irradiance=None to the method signature and drop kwargs? I don't see one, but I feel like I might be forgetting something from a previous discussion.

I think I was anticipating new models with new weather inputs, but better to cross that bridge if we come to it. The code is much simpler now without this.

wholmgren

Thanks @kanderso-nrel. @cwhanse can you take another look, especially double checking the handling of effective_irradiance? Ok with me to merge if you approve.

cwhanse

It looks to me that ModelChain.temperature_model becomes obsolete with addition of these methods. I could be mistaken. Separate issue, in either case.

kandersolar added 4 commits April 14, 2021 20:30

add Array.get_cell_temperature and PVSystem.get_cell_temperature, dep…

6b69027

…recate old PVSystem tcell methods, update ModelChain._set_celltemp

fix tests

6b7dcff

api.rst

df1e712

whatsnew

3b850b0

kandersolar added the api label Apr 15, 2021

kandersolar added this to the 0.9.0 milestone Apr 15, 2021

add test for invalid model string

42691d1

kandersolar added 2 commits April 14, 2021 20:58

stickler, docs

e44cc26

nudge to trigger CI

e744b04

wholmgren approved these changes Apr 21, 2021

View reviewed changes

cwhanse self-requested a review April 21, 2021 14:46

cwhanse reviewed Apr 26, 2021

View reviewed changes

fix whatsnew

ec7b5fe

wholmgren reviewed Apr 27, 2021

View reviewed changes

cwhanse mentioned this pull request May 5, 2021

pvlib v0.9 release planning #1214

Closed

24 tasks

kandersolar and others added 7 commits May 11, 2021 20:04

docstring fix

4c03c93

try out kinder error messages

97e400d

Merge branch 'master' into array_tcell

0a2fad5

fixing bugs

2c0cd00

test for nice error message

cb85661

Update pvlib/pvsystem.py

f01b9c1

Co-authored-by: Cliff Hansen <[email protected]>

stickler

7748074

cwhanse reviewed May 12, 2021

View reviewed changes

pvlib/modelchain.py Show resolved Hide resolved

cwhanse reviewed May 12, 2021

View reviewed changes

pvlib/pvsystem.py Outdated Show resolved Hide resolved

cwhanse reviewed May 12, 2021

View reviewed changes

pvlib/pvsystem.py Outdated Show resolved Hide resolved

kandersolar and others added 4 commits May 12, 2021 10:13

Update pvlib/modelchain.py

4f03b08

Co-authored-by: Cliff Hansen <[email protected]>

Merge remote-tracking branch 'upstream/master' into array_tcell

7668b38

Merge branch 'array_tcell' of https://github.com/kanderso-nrel/pvlib-…

e5c43cc

…python into array_tcell

move _build_args to pvlib.tools

0e59b30

wholmgren reviewed May 14, 2021

View reviewed changes

kandersolar added 5 commits May 14, 2021 07:47

remove deprecated items from api.rst

d538037

def -> partial

b97c045

docstring edits

bb08d62

see also fixes

f50c7fe

remove kwarg nonsense

98c6776

wholmgren approved these changes May 14, 2021

View reviewed changes

cwhanse approved these changes May 14, 2021

View reviewed changes

wholmgren merged commit 77ac247 into pvlib:master May 14, 2021

kandersolar deleted the array_tcell branch May 14, 2021 17:14

Add Array.get_cell_temperature, PVSystem.get_cell_temperature #1211

Add Array.get_cell_temperature, PVSystem.get_cell_temperature #1211

Uh oh!

Conversation

kandersolar commented Apr 15, 2021

Uh oh!

kandersolar commented Apr 15, 2021

Uh oh!

wholmgren left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kandersolar commented May 14, 2021

Uh oh!

wholmgren left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wholmgren left a comment

Choose a reason for hiding this comment

Uh oh!

cwhanse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!