[ENH] extend iam.physical with optional n_ar parameter for AR coating #1616

kdebrab · 2022-12-16T11:52:35Z

Closes IAM that supports AR coating like Fresnel #1501
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.

mikofski · 2022-12-17T05:55:30Z

I love this so much! I’ll try to take a look asap unless someone else beats me to it. Thank you!

adriesse

Overall I like this code, but I think it would be better if all the arguments could be vectors like in the original version.

mikofski · 2023-01-18T21:29:19Z

it looks like there's a stickler alert, some long lines, can you clean those up?

adriesse · 2023-01-18T21:31:08Z

@mikofski Beat you to it by 15 minutes! :)

kdebrab · 2023-02-01T09:48:20Z

Overall I like this code, but I think it would be better if all the arguments could be vectors like in the original version.

Unless I'm mistaken, aoi, K and L could already be vectors, but the code will indeed fail when n or n_ar is a vector.

So, you mean that the code should also support n and n_ar as vectors?

adriesse · 2023-02-01T09:53:08Z

So, you mean that the code should also support n and n_ar as vectors?

Yes, that's what I was thinking, if it's not too complicated.

mikofski · 2023-02-01T16:23:07Z

If making them vectors is a bridge too far, I’d prefer bird in the hand instead of two in the bush.

what is the use case for vector forms of n & n_ar and how often?

let’s get this pull request closed and follow up with a refinement.

don’t let perfect be the enemy of good

@kdebrab can you address the pep-8 issues? I’d like to merge this ASAP if there are no other concerns. Thanks!

adriesse · 2023-02-01T16:36:11Z

I’d like to merge this ASAP

We do need some reviews/reviewers...

kdebrab · 2023-02-01T19:55:10Z

Personally, I'm not convinced whether we should really support n and n_ar as vectors. Also, I'm wondering whether we shouldn't move to the now ubiquitous black code style, which (amongst others) defaults to a line length of 88 instead of 79 characters.

Nevertheless, I pushed an update which addresses both requests. n and n_ar as vectors are now supported and the "E501 line too long" stickler alerts have been resolved.

adriesse · 2023-02-02T08:58:50Z

Great, thanks! Interesting to learn about black too. A use case for n/n_ar as vectors could be modeling an ar coating that degrades. In research mode I also like to send a vectors to do parameter sweeps on models.

AdamRJensen

A quick first partial review from my side.

pvlib/iam.py

AdamRJensen · 2023-02-02T09:11:32Z

pvlib/iam.py

@@ -91,21 +91,22 @@ def ashrae(aoi, b=0.05):
    return iam


-def physical(aoi, n=1.526, K=4., L=0.002):
+def physical(aoi, n=1.526, K=4.0, L=0.002, *, n_ar=None):


What's the reasoning behind including an asterisk (allowing a variable number of arguments) in this specific function?

The ‘single star’ syntax indicates the end of positional parameters and enforces the user to use a keyword for all following parameters, see https://peps.python.org/pep-3102.
In other words physical(aoi, 1.526, 4.0, 0.0032, 1.29) is not allowed and will fail with a TypeError. One needs to explicitly add the n_ar keyword, like physical(aoi, 1.526, 4.0, 0.0032, n_ar=1.29).

I think of using * to be good practice for functions with many parameters, so I typically do it when adding new parameters, but it may be overkill. On the other hand, removing the asterisk later is easy and doesn't break anything, but adding it later would be a breaking change.

Huh, interesting, I certainly didn't know that. I'm all for keeping it. Thanks for the explanation.

I think it will be a first for pvlib, but I'm okay with keeping it. If anyone complains we can always revert it as @kdebrab points out.

adriesse · 2023-02-02T10:30:50Z

@AdamRJensen Did you run black on this file?

kdebrab · 2023-02-02T11:53:17Z

@AdamRJensen Did you run black on this file?

Oh, my mistake. I accidentally applied black in my latest commit. I reverted that commit now.

adriesse · 2023-02-02T19:48:00Z

Interestingly black did not make all that many changes. I intend to throw some of my personal code at it and fear it may crash!

adriesse · 2023-02-02T19:54:26Z

@kdebrab I could test this, but you probably have done so already: if you leave out the if statements, do the calculations produce identical results with n_ar=1 and n_ar=n as inputs? The purpose of the if statements is just to save some processing time, right?

kdebrab · 2023-02-03T09:42:40Z

@kdebrab I could test this, but you probably have done so already: if you leave out the if statements, do the calculations produce identical results with n_ar=1 and n_ar=n as inputs? The purpose of the if statements is just to save some processing time, right?

Indeed, the only purpose of the 'if not np.allclose(n3, n2):' statement is to save processing time in case there is no AR coating. When n2==n3, sintheta and costheta don't change, the rho23 components are zero and as a result the tau components don't change.

As requested, I double checked: after out-commenting the line if not np.allclose(n3, n2): (and shifting the subsequent block to the left), the tests passed and thus the results (see test_physical and test_physical_noar) are indeed identical.

[EDIT]
I see that I may have misunderstood your question: you probably mean out-commenting the complete if block. In that case, the only test that fails is test_physical_ar (for obvious reasons), but all other tests pass, and thus for n_ar=1 and n_ar=n, results remain the same.

kandersolar

A couple minor comments but otherwise LGTM

kandersolar · 2023-02-03T14:01:22Z

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

@@ -17,6 +17,8 @@ Deprecations
 Enhancements
 ~~~~~~~~~~~~

+* Added optional ``n_ar`` parameter to :py:func:`pvlib.iam.physical` to
+  support an anti-reflective coating. (:issue:`1501`, :pull:`1616`)


Don't forget to add yourself to the Contributors list below :)

As well as the reviewers.

kandersolar · 2023-02-03T16:23:20Z

pvlib/iam.py

@@ -121,6 +122,10 @@ def physical(aoi, n=1.526, K=4., L=0.002):
        indicates that 0.002 meters (2 mm) is reasonable for most
        glass-covered PV panels.

+    n_ar : numeric, default None


Suggested change

n_ar : numeric, default None

n_ar : numeric, optional

See #1574

I'm okay with the default being normal glass, no ARC, but would it be possible in the docstring somewhere, either here or after returns in the notes, to suggest nAR = 1.290 to mimic PVsyst? Or to provide a reference of what would be typical values?

I updated the docstring.

kandersolar · 2023-02-03T16:31:26Z

pvlib/iam.py

@@ -91,21 +91,22 @@ def ashrae(aoi, b=0.05):
    return iam


-def physical(aoi, n=1.526, K=4., L=0.002):
+def physical(aoi, n=1.526, K=4.0, L=0.002, *, n_ar=None):


I think it will be a first for pvlib, but I'm okay with keeping it. If anyone complains we can always revert it as @kdebrab points out.

adriesse · 2023-02-03T20:35:52Z

As requested, I double checked:

Thanks @kdebrab !

adriesse

Nice work!

mikofski · 2023-02-03T20:56:59Z

pvlib/iam.py

@@ -121,6 +122,10 @@ def physical(aoi, n=1.526, K=4., L=0.002):
        indicates that 0.002 meters (2 mm) is reasonable for most
        glass-covered PV panels.

+    n_ar : numeric, default None


I'm okay with the default being normal glass, no ARC, but would it be possible in the docstring somewhere, either here or after returns in the notes, to suggest nAR = 1.290 to mimic PVsyst? Or to provide a reference of what would be typical values?

pvlib/iam.py

pvlib/tests/test_iam.py

mikofski · 2023-02-04T00:31:14Z

@kdebrab either in this PR or in a follow up I also think there should be a test for vector n and n_ar and an example in the Gallery to show how to use this function that way, especially if the shape of the inputs is important, EG: should aoi be in the 1st dimension and n in the 2nd dimension? Thanks! PS sorry if this is redundant with my previous comment. I can't find it now.

kdebrab · 2023-02-04T14:35:53Z

@kdebrab either in this PR or in a follow up I also think there should be a test for vector n and n_ar and an example in the Gallery to show how to use this function that way, especially if the shape of the inputs is important, EG: should aoi be in the 1st dimension and n in the 2nd dimension? Thanks! PS sorry if this is redundant with my previous comment. I can't find it now.

There is indeed no test against vectors of n, n_ar, L or K. I think when @adriesse uses vectors, these have the same dimension as aoi. Maybe, @adriesse can add a test for that in a follow up PR?

mikofski · 2023-02-04T15:52:37Z

Thanks @kdebrab and all reviewers!

[ENH] extend iam.physical with optional n_ar parameter for AR coating

70c0493

kdebrab mentioned this pull request Dec 16, 2022

IAM that supports AR coating like Fresnel #1501

Closed

adriesse requested changes Jan 18, 2023

View reviewed changes

[CLN] resolve stickler-ci failures; [ENH] ensure inputs could be vectors

e54ff70

kdebrab and others added 3 commits February 1, 2023 21:09

Merge branch 'pvlib:main' into iam_for_AR_coating

e58fdb6

[DOC] iam.physical: update docstring and add whatsnew entry

f3a82dd

[CLN] minor non-functional change to improve readability

c0ba45a

AdamRJensen reviewed Feb 2, 2023

View reviewed changes

AdamRJensen added the enhancement label Feb 2, 2023

revert latest commit

ab07267

apply suggested changes

f295763

kandersolar approved these changes Feb 3, 2023

View reviewed changes

adriesse approved these changes Feb 3, 2023

View reviewed changes

mikofski approved these changes Feb 3, 2023 •

edited

Loading

View reviewed changes

update docstring, add contributors

14cd046

mikofski merged commit a19b528 into pvlib:main Feb 4, 2023

kdebrab deleted the iam_for_AR_coating branch February 25, 2023 21:46

kdebrab mentioned this pull request Mar 24, 2023

[FIX] iam.physical returns nan for aoi > 90° when n = 1 (#1706) #1707

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] extend iam.physical with optional n_ar parameter for AR coating #1616

[ENH] extend iam.physical with optional n_ar parameter for AR coating #1616

kdebrab commented Dec 16, 2022 •

edited by AdamRJensen

Loading

mikofski commented Dec 17, 2022

adriesse left a comment

mikofski commented Jan 18, 2023

adriesse commented Jan 18, 2023 •

edited

Loading

kdebrab commented Feb 1, 2023

adriesse commented Feb 1, 2023

mikofski commented Feb 1, 2023

adriesse commented Feb 1, 2023

kdebrab commented Feb 1, 2023

adriesse commented Feb 2, 2023

AdamRJensen left a comment

AdamRJensen Feb 2, 2023

kdebrab Feb 2, 2023

AdamRJensen Feb 2, 2023

kandersolar Feb 3, 2023

adriesse commented Feb 2, 2023

kdebrab commented Feb 2, 2023

adriesse commented Feb 2, 2023

adriesse commented Feb 2, 2023

kdebrab commented Feb 3, 2023 •

edited

Loading

kandersolar left a comment

kandersolar Feb 3, 2023

adriesse Feb 3, 2023

kandersolar Feb 3, 2023

mikofski Feb 3, 2023

kdebrab Feb 4, 2023

kandersolar Feb 3, 2023

adriesse commented Feb 3, 2023

adriesse left a comment

mikofski Feb 3, 2023

mikofski commented Feb 4, 2023

kdebrab commented Feb 4, 2023 •

edited

Loading

mikofski commented Feb 4, 2023

[ENH] extend iam.physical with optional n_ar parameter for AR coating #1616

[ENH] extend iam.physical with optional n_ar parameter for AR coating #1616

Conversation

kdebrab commented Dec 16, 2022 • edited by AdamRJensen Loading

mikofski commented Dec 17, 2022

adriesse left a comment

Choose a reason for hiding this comment

mikofski commented Jan 18, 2023

adriesse commented Jan 18, 2023 • edited Loading

kdebrab commented Feb 1, 2023

adriesse commented Feb 1, 2023

mikofski commented Feb 1, 2023

adriesse commented Feb 1, 2023

kdebrab commented Feb 1, 2023

adriesse commented Feb 2, 2023

AdamRJensen 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

adriesse commented Feb 2, 2023

kdebrab commented Feb 2, 2023

adriesse commented Feb 2, 2023

adriesse commented Feb 2, 2023

kdebrab commented Feb 3, 2023 • edited Loading

kandersolar 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

adriesse commented Feb 3, 2023

adriesse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikofski commented Feb 4, 2023

kdebrab commented Feb 4, 2023 • edited Loading

mikofski commented Feb 4, 2023

kdebrab commented Dec 16, 2022 •

edited by AdamRJensen

Loading

adriesse commented Jan 18, 2023 •

edited

Loading

kdebrab commented Feb 3, 2023 •

edited

Loading

kdebrab commented Feb 4, 2023 •

edited

Loading