Add Prilliman et al transience model to pvlib.temperature #1391

kandersolar · 2022-01-25T02:44:13Z

Closes implement transient moving-average thermal model #1081
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.

This PR adds a cleaned-up version of prilliman_v5 from this notebook: https://gist.github.com/kanderso-nrel/1d6da384d7af8afc24c230f1f144eb57

Also: I noticed that one of the links in the PR template is out of date so I updated that here too.

kandersolar · 2022-01-26T02:21:12Z

Ready for review.

@mjprilliman comments are welcome :)

cwhanse

Haven't looked at the test details yet

pvlib/temperature.py

cwhanse · 2022-01-28T21:13:37Z

pvlib/temperature.py

+
+    time_step = (temp_cell.index[1] - temp_cell.index[0]).total_seconds()
+    if time_step >= 1200:
+        # too coarsely sampled for smoothing to be relevant


I think a warning here would be appropriate

Maybe extend the message to say that the return values are the original temperature?

pvlib/temperature.py

pvlib/tests/test_temperature.py

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

pvlib/temperature.py

Co-authored-by: Anton Driesse <[email protected]>

wholmgren · 2022-02-03T17:05:36Z

Great to see this. I'll review soon.

Reproducing something like Figure 5 from the reference would be a great addition to the example library.

wholmgren

Looks good! All minor (hopefully) comments below.

pvlib/temperature.py

pvlib/tests/test_temperature.py

pvlib/temperature.py

mjprilliman

I'm not familiar enough with pvlib to give a full review but the function implementation looks good to me. Thanks @kanderso-nrel !

Co-authored-by: Will Holmgren <[email protected]>

kandersolar · 2022-02-03T21:51:22Z

Thanks @wholmgren for the comments, was just lazy of me to not document that numpy magic. I came up with it and still had to remind myself how it worked!

wholmgren

Any further comments or should we merge?

cwhanse · 2022-02-03T21:54:51Z

I'd like to take one more look.

cwhanse

You win some unnamed award for excellent in-line comments

pvlib/temperature.py

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

kandersolar · 2022-02-04T00:18:55Z

Actually I think there may be a flaw in this implementation regarding treatment of nans. Please do not merge yet!

kandersolar · 2022-02-04T01:11:39Z

Ok, fixed. The problem was that NaNs in the input temperature were correctly ignored in the numerator (np.nansum(subsets * weights, axis=1)), but the corresponding weights were not set to zero in the denominator, so in effect it was treating NaN temperature as zero temperature instead of ignoring it. The fix was to replace the triu incantation, which properly ignored NaNs for the "prefix", with a simple mask that ignores all NaNs, not just the prefix. So the code is simpler now too.

I don't think this should be controversial but it's probably worth another pair of eyes. Thanks!

cwhanse · 2022-02-04T01:38:29Z

pvlib/temperature.py

+    value. At the beginning of the series where a full 20 minute window is not
+    possible, partial windows are used instead.
+
+    References


One more suggestion: explain in Notes how nan in inputs corresponds to nan in output. It makes sense (to me) that output at k is nan if windspeed[k] is nan. As a user, I may be surprised non-nan at position k in output when input temp_cell[k] is nan. But that also makes sense, because the non-nan value can be produced when window of temp_cell prior to k has data.

Output ``temp_cell[k]`` is NaN when input ``wind_speed[k]`` is NaN, or when no non-NaN data are in the input temperature for the 20 minute window preceding index ``k``.

adriesse · 2022-02-04T12:49:04Z

You win some unnamed award for excellent in-line comments

I had noticed these also, with appreciation.

adriesse · 2022-02-04T14:37:37Z

pvlib/temperature.py

+    -------
+    temp_cell : pandas.Series
+        Smoothed version of the input cell temperature [C]
+


Add here that the original data is returned if too coarsely sampled.

adriesse · 2022-02-04T14:38:01Z

pvlib/temperature.py

+
+    time_step = (temp_cell.index[1] - temp_cell.index[0]).total_seconds()
+    if time_step >= 1200:
+        # too coarsely sampled for smoothing to be relevant


Maybe extend the message to say that the return values are the original temperature?

cwhanse · 2022-02-06T17:53:48Z

If no objections, will merge tomorrow.

wholmgren · 2022-02-16T19:40:41Z

pvlib/temperature.py

+    # np.sum for denominator to propagate nans in input wind speed.
+    numerator = np.nansum(subsets * weights, axis=1)
+    denominator = np.sum(weights, axis=1)
+    smoothed = numerator / denominator


@kanderso-nrel this line emitted a warning despite the comment on lines 967-969 that suggested otherwise. Is this a version-specific issue? My versions are below...

INSTALLED VERSIONS ------------------ commit : 66e3805b8cabe977f40c05259cc3fcf7ead5687d python : 3.9.7.final.0 python-bits : 64 OS : Windows OS-release : 10 Version : 10.0.18363 machine : AMD64 processor : Intel64 Family 6 Model 140 Stepping 1, GenuineIntel byteorder : little LC_ALL : None LANG : None LOCALE : English_United States.1252 pandas : 1.3.5 numpy : 1.20.3

Hmm, the only way I can reproduce this is if there is a stretch of NaN in the input lasting longer than 20 minutes -- is that true for your case? That comment is referring to a div by zero warning cause by a quirk of this implementation (no weights for the very first value), not a div by zero warning caused by nans in the input.

Ah, thanks. Yes, I had 38 minutes of NaN temperatures. I filled only the middle point with a valid number and the warning disappeared.

kandersolar added 5 commits January 24, 2022 21:28

create temperature.prilliman

7619d04

api.rst

ff2c006

whatsnew

95bff13

Update PULL_REQUEST_TEMPLATE.md

e5f83db

fix comment

f92e73a

kandersolar added the enhancement label Jan 25, 2022

kandersolar added this to the 0.9.1 milestone Jan 25, 2022

kandersolar added 4 commits January 24, 2022 21:45

stickler

92afe49

a bit of cleanup

56519e2

tests

c384415

fix whatsnew

d92250c

kandersolar marked this pull request as ready for review January 26, 2022 02:13

cwhanse reviewed Jan 28, 2022

View reviewed changes

kandersolar and others added 3 commits January 29, 2022 10:43

Apply suggestions from code review

985baba

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

stickler

01c4afc

use _get_sample_intervals; add warning

188e3df

adriesse reviewed Feb 3, 2022

View reviewed changes

pvlib/temperature.py Outdated Show resolved Hide resolved

Update pvlib/temperature.py

be1704e

Co-authored-by: Anton Driesse <[email protected]>

wholmgren reviewed Feb 3, 2022

View reviewed changes

mjprilliman reviewed Feb 3, 2022

View reviewed changes

kandersolar and others added 3 commits February 3, 2022 14:06

Apply suggestions from code review

5e2c6da

Co-authored-by: Will Holmgren <[email protected]>

move _get_sample_intervals to pvlib.tools

2197f3c

update temperature.prilliman from review

3095afb

wholmgren approved these changes Feb 3, 2022

View reviewed changes

cwhanse approved these changes Feb 3, 2022

View reviewed changes

pvlib/temperature.py Outdated Show resolved Hide resolved

Update pvlib/temperature.py

7cecbbe

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

fix weights bug for nans; simplify

5fda720

cwhanse reviewed Feb 4, 2022

View reviewed changes

add Note about nans

b8d5479

adriesse reviewed Feb 4, 2022

View reviewed changes

update docstring and warning text

0a0a1f3

cwhanse merged commit f86d71d into pvlib:master Feb 8, 2022

kandersolar deleted the prilliman branch February 8, 2022 12:52

wholmgren reviewed Feb 16, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Prilliman et al transience model to pvlib.temperature #1391

Add Prilliman et al transience model to pvlib.temperature #1391

kandersolar commented Jan 25, 2022 •

edited

Loading

kandersolar commented Jan 26, 2022

cwhanse left a comment

cwhanse Jan 28, 2022

adriesse Feb 4, 2022

wholmgren commented Feb 3, 2022

wholmgren left a comment

mjprilliman left a comment •

edited

Loading

kandersolar commented Feb 3, 2022

wholmgren left a comment

cwhanse commented Feb 3, 2022

cwhanse left a comment

kandersolar commented Feb 4, 2022

kandersolar commented Feb 4, 2022

cwhanse Feb 4, 2022

adriesse commented Feb 4, 2022

adriesse Feb 4, 2022

adriesse Feb 4, 2022

cwhanse commented Feb 6, 2022 •

edited

Loading

wholmgren Feb 16, 2022

kandersolar Feb 16, 2022

wholmgren Feb 23, 2022

Add Prilliman et al transience model to pvlib.temperature #1391

Add Prilliman et al transience model to pvlib.temperature #1391

Conversation

kandersolar commented Jan 25, 2022 • edited Loading

kandersolar commented Jan 26, 2022

cwhanse 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 commented Feb 3, 2022

wholmgren left a comment

Choose a reason for hiding this comment

mjprilliman left a comment • edited Loading

Choose a reason for hiding this comment

kandersolar commented Feb 3, 2022

wholmgren left a comment

Choose a reason for hiding this comment

cwhanse commented Feb 3, 2022

cwhanse left a comment

Choose a reason for hiding this comment

kandersolar commented Feb 4, 2022

kandersolar commented Feb 4, 2022

Choose a reason for hiding this comment

adriesse commented Feb 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwhanse commented Feb 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kandersolar commented Jan 25, 2022 •

edited

Loading

mjprilliman left a comment •

edited

Loading

cwhanse commented Feb 6, 2022 •

edited

Loading