Implement enhanced Faiman temperature model #1595

adriesse · 2022-11-17T16:41:11Z

Motivated by Create new temperature model(s) incorporating a simple radiative loss term. #1594
I am familiar with the contributing guidelines
Tests added
Updates entries in docs/sphinx/source/reference 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 (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

adriesse · 2022-11-17T16:42:31Z

Early comments welcome, as always! @jsstein @mtheristis

adriesse · 2022-11-22T15:56:55Z

Ready for review. I'll do the sphinx stuff when it is clear which release this goes into. (The upcoming one I hope.)

cwhanse

I think we should have a test that checks the output of this function matches the output of faiman, for default parameters.

pvlib/temperature.py

cwhanse · 2022-11-22T16:22:17Z

pvlib/temperature.py

+        :math:`\left[ \frac{\text{W}/\text{m}^2}{\text{C}\ \left( \text{m/s} \right)} \right]`
+
+    sky_view : numeric, default 1.0
+        Effective view factor limiting the radiative exchange between the


Is "effective" adding something here? sky_view looks to be the fraction of the sky dome "seen" by the module, so that's the usual view factor. I wonder if the formula could be considered part of the model and the input would be surface_tilt, but I'll defer to the model author.

I snuck in the "effective" because the infra-red sky is very non-isotropic and that needs to be taken into account. The formula is really just a starting point for future work, not adequately explored and validated.

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

adriesse · 2022-11-22T20:15:02Z

I think we should have a test that checks the output of this function matches the output of faiman, for default parameters.

You mean, the test should call faiman to determine the expected value?

cwhanse · 2022-11-22T20:15:38Z

I think we should have a test that checks the output of this function matches the output of faiman, for default parameters.

You mean, the test should call faiman to determine the expected value?

Yes

adriesse · 2022-11-29T17:02:00Z

Additional reviewer(s) invited!

kandersolar

@adriesse I've tagged this for 0.9.4 if you're ready to add a what'snew entry for this PR

pvlib/temperature.py

adriesse · 2022-12-01T23:42:50Z

Last chance to comment! @jsstein @mtheristis

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

pvlib/temperature.py

mtheristis · 2022-12-02T16:13:48Z

@adriesse, I am sorry for the delay.

Out of curiosity, is there a reason we are only focusing on Faiman here? Or will the others (SAPM, NOCT, PVSyst) come later?

adriesse · 2022-12-02T16:37:56Z

@adriesse, I am sorry for the delay.

Out of curiosity, is there a reason we are only focusing on Faiman here? Or will the others (SAPM, NOCT, PVSyst) come later?

Good questions. There were multiple considerations in my head. I decided to do one in order to keep the PR to a reasonable size and work through integration issues on one function rather than four at a time. I picked Faiman because it is the simplest but also because it is used in IEC 61853. When this feature is released I will point it out to interested parties in the working group for that standard.

If there is user demand I see no problem to implement the other three and maybe enhance the generic linear model too.

mtheristis · 2022-12-02T16:48:59Z

@adriesse, I am sorry for the delay.
Out of curiosity, is there a reason we are only focusing on Faiman here? Or will the others (SAPM, NOCT, PVSyst) come later?

Good questions. There were multiple considerations in my head. I decided to do one in order to keep the PR to a reasonable size and work through integration issues on one function rather than four at a time. I picked Faiman because it is the simplest but also because it is used in IEC 61853. When this feature is released I will point it out to interested parties in the working group for that standard.

If there is user demand I see no problem to implement the other three and maybe enhance the generic linear model too.

Understood.

I am just thinking that we mind end up with X versions of a same base model, and the users might be confused with their differences. Is there a way to have an option when running faiman to say "include_radiative_losses = True/False"? Or is this bad practice?

adriesse · 2022-12-02T17:08:22Z

I am just thinking that we mind end up with X versions of a same base model, and the users might be confused with their differences.

Well, it has taken a lot of years for the first enhancement to arrive and I'm not planning another one! Presenting this as an enhancement of something that people feel familiar and comfortable with is a strategy to gain acceptance more readily. It could
easily have been packaged as a single new model.

Is there a way to have an option when running faiman to say "include_radiative_losses = True/False"? Or is this bad practice?

Perhaps in the system class or model chain something like this is possible. I'm not sure.

AdamRJensen

Cool stuff and nice investigations :)

pvlib/temperature.py

pvlib/tests/test_temperature.py

pvlib/temperature.py

adriesse

Thanks for reviewing, Adam. I'm glad you found the work interesting!
I responded with some comments and will hold off on code changes to see what other comments might appear...

PS. Sorry, I thought these would appear as replies to your comments. :(
PPS. Oh they do show up as replies. They're above as well as below! :)

pvlib/temperature.py

pvlib/tests/test_temperature.py

AdamRJensen · 2022-12-07T12:19:16Z

pvlib/temperature.py

+    u0 = np.asanyarray(u0)
+    u1 = np.asanyarray(u1)
+    emissivity = np.asanyarray(emissivity)
+
+    abs_zero = np.array(-273.15)
+    sigma = np.array(scipy.constants.Stefan_Boltzmann)
+
+    if ir_down is None:
+        qrad_sky = np.array(0.0)


Suggested change

u0 = np.asanyarray(u0)

u1 = np.asanyarray(u1)

emissivity = np.asanyarray(emissivity)

abs_zero = np.array(-273.15)

sigma = np.array(scipy.constants.Stefan_Boltzmann)

if ir_down is None:

qrad_sky = np.array(0.0)

poa_global = np.asanyarray(poa_global)

temp_air = np.asanyarray(temp_air)

wind_speed = np.asanyarray(wind_speed)

ir_down = np.asanyarray(ir_down)

u0 = np.asanyarray(u0)

u1 = np.asanyarray(u1)

sky_view = np.asanyarray(sky_view)

emissivity = np.asanyarray(emissivity)

abs_zero = -273.15

sigma = scipy.constants.Stefan_Boltzmann

if ir_down is None:

qrad_sky = 0.0

I see that the usage of np.asanyarray is very carefully chosen such that all inputs can be passed as lists and directly or indirectly be converted to arrays. The indirect conversion to arrays (by multiplying a list something array-like) seems undesirable and unintuitive to the user. Why not instead just convert all the inputs to array-like in the beginning and be straightforward about it?

It's not a blocker for me, but this is the point that I was trying to raise earlier.

On the whole I believe this function behaves like numpy would. The reason for the indirect and partial approach is to allow pandas and xarray data structures to be propagated from the first four arguments. Try it, you might like it!

What about this function requires any of the np.asanyarray calls? np.asanyarray is not robust.

Ah, now I see the hidden conversation about allowing list input. That is not needed and comes at a cost of disallowing arbitrary array-like input that doesn't fully support the asanyarray machinery (e.g. dask, or whatever the pydata library of the month is).

Could you elaborate on what doesn't work here?

Overall I'm getting more push-back than expected on something which I though was a very simple technique to make the function very flexible. If there is a concrete down-side I'd like to learn about it, especially the risks associated with asanyarray (which could be replaced with asarray in this particular function). I've use this technique before and am likely to use it again unless there really is a problem with it.

The function is already very flexible because you did a good job writing it :). It doesn't need asanyarray! Here's a concrete downside for asanyarray:

In [1]: import numpy as np In [2]: import dask.array as da In [3]: da.from_array([0, 1]) Out[3]: dask.array<array, shape=(2,), dtype=int32, chunksize=(2,), chunktype=numpy.ndarray> In [4]: np.asanyarray(da.from_array([0, 1])) Out[4]: array([0, 1]) In [5]: np.asarray(da.from_array([0, 1])) Out[5]: array([0, 1])

np.asanyarray is not robust.

I am curious to learn more about this...

Hi Anton, here's a quote from NEP16

Sometimes people suggest using np.asanyarray for this purpose, but unfortunately its semantics are exactly backwards: it guarantees that the object it returns uses the same memory layout as an ndarray, but tells you nothing at all about its semantics, which makes it essentially impossible to use safely in practice.

The scientific python brain trust has discussed solutions in a variety of venues and I find it very difficult to track, but here's a very incomplete list of links:

https://numpy.org/neps/nep-0022-ndarray-duck-typing-overview.html

https://labs.quansight.org/blog/2021/11/pydata-extensibility-vision

https://discuss.scientific-python.org/t/spec-2-api-dispatch/173

I think we have another issue or two related to this, so probably best to continue discussion there rather than a closed PR :)

Thanks! That was educational!

adriesse · 2022-12-12T18:13:33Z

Thank for all the reviews!!

New faiman_rad function. No tests yet.

54dcae1

adriesse added 2 commits November 20, 2022 11:23

Adjust arguments and default behaviour.

c055913

Improve doc string, add tests, fix some old tests.

2608b8d

cwhanse added the enhancement label Nov 22, 2022

cwhanse reviewed Nov 22, 2022

View reviewed changes

Update pvlib/temperature.py

176f917

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

adriesse added 2 commits November 22, 2022 21:29

Minor improvements here and there.

04a5827

Add entry in pv_modeling.rst

110299b

cwhanse approved these changes Nov 29, 2022

View reviewed changes

kandersolar added this to the 0.9.4 milestone Dec 1, 2022

kandersolar reviewed Dec 1, 2022

View reviewed changes

pvlib/temperature.py Outdated Show resolved Hide resolved

pvlib/temperature.py Outdated Show resolved Hide resolved

pvlib/temperature.py Show resolved Hide resolved

adriesse added 4 commits December 1, 2022 22:17

Update whatsnew and touch up doc strings.

a51f840

Merge branch 'main' into rad-loss

e9dde82

Enhance reference in doc string.

ef05a73

Add DOI.

eafb272

kandersolar approved these changes Dec 2, 2022

View reviewed changes

docs/sphinx/source/whatsnew/v0.9.4.rst Outdated Show resolved Hide resolved

pvlib/temperature.py Outdated Show resolved Hide resolved

Finishing touches.

a6e93d0

Merge branch 'pvlib:main' into rad-loss

b470257

AdamRJensen reviewed Dec 5, 2022

View reviewed changes

adriesse commented Dec 5, 2022

View reviewed changes

pvlib/temperature.py Outdated Show resolved Hide resolved

pvlib/temperature.py Show resolved Hide resolved

pvlib/temperature.py Outdated Show resolved Hide resolved

pvlib/temperature.py Outdated Show resolved Hide resolved

pvlib/tests/test_temperature.py Outdated Show resolved Hide resolved

adriesse added 2 commits December 6, 2022 20:47

Merge branch 'pvlib:main' into rad-loss

91f7086

Address review comments.

bd05c35

AdamRJensen reviewed Dec 7, 2022

View reviewed changes

AdamRJensen self-requested a review December 7, 2022 12:20

AdamRJensen approved these changes Dec 7, 2022

View reviewed changes

kandersolar mentioned this pull request Dec 9, 2022

v0.9.4 release plan #1608

Closed

9 tasks

adriesse and others added 2 commits December 11, 2022 21:37

Whatever it takes to get merged.

bae186a

Merge branch 'main' into rad-loss

bda2971

AdamRJensen merged commit 4bdbfd6 into pvlib:main Dec 12, 2022

kandersolar mentioned this pull request Dec 12, 2022

Create new temperature model(s) incorporating a simple radiative loss term. #1594

Closed

adriesse deleted the rad-loss branch December 12, 2022 18:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement enhanced Faiman temperature model #1595

Implement enhanced Faiman temperature model #1595

adriesse commented Nov 17, 2022 •

edited

Loading

adriesse commented Nov 17, 2022 •

edited

Loading

adriesse commented Nov 22, 2022

cwhanse left a comment

cwhanse Nov 22, 2022

adriesse Nov 22, 2022

adriesse commented Nov 22, 2022

cwhanse commented Nov 22, 2022

adriesse commented Nov 29, 2022

kandersolar left a comment

adriesse commented Dec 1, 2022

mtheristis commented Dec 2, 2022

adriesse commented Dec 2, 2022

mtheristis commented Dec 2, 2022

adriesse commented Dec 2, 2022

AdamRJensen left a comment

adriesse left a comment •

edited

Loading

AdamRJensen Dec 7, 2022 •

edited

Loading

adriesse Dec 7, 2022

wholmgren Dec 7, 2022 •

edited

Loading

wholmgren Dec 7, 2022

adriesse Dec 7, 2022

adriesse Dec 8, 2022

wholmgren Dec 8, 2022

adriesse Dec 9, 2022

wholmgren Dec 12, 2022

adriesse Dec 12, 2022

adriesse commented Dec 12, 2022

Implement enhanced Faiman temperature model #1595

Implement enhanced Faiman temperature model #1595

Conversation

adriesse commented Nov 17, 2022 • edited Loading

adriesse commented Nov 17, 2022 • edited Loading

adriesse commented Nov 22, 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

adriesse commented Nov 22, 2022

cwhanse commented Nov 22, 2022

adriesse commented Nov 29, 2022

kandersolar left a comment

Choose a reason for hiding this comment

adriesse commented Dec 1, 2022

mtheristis commented Dec 2, 2022

adriesse commented Dec 2, 2022

mtheristis commented Dec 2, 2022

adriesse commented Dec 2, 2022

AdamRJensen left a comment

Choose a reason for hiding this comment

adriesse left a comment • edited Loading

Choose a reason for hiding this comment

AdamRJensen Dec 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wholmgren Dec 7, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriesse commented Dec 12, 2022

adriesse commented Nov 17, 2022 •

edited

Loading

adriesse commented Nov 17, 2022 •

edited

Loading

adriesse left a comment •

edited

Loading

AdamRJensen Dec 7, 2022 •

edited

Loading

wholmgren Dec 7, 2022 •

edited

Loading