8000 [ENH] extend iam.physical with optional n_ar parameter for AR coating by kdebrab · Pull Request #1616 · pvlib/pvlib-python · GitHub
[go: up one dir, main page]
Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Feb 4, 2023
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
70c0493
[ENH] extend iam.physical with optional n_ar parameter for AR coating
kdebrab Dec 4, 2022
e54ff70
[CLN] resolve stickler-ci failures; [ENH] ensure inputs could be vectors
kdebrab Feb 1, 2023
e58fdb6
Merge branch 'pvlib:main' into iam_for_AR_coating
kdebrab Feb 1, 2023
f3a82dd
[DOC] iam.physical: update docstring and add whatsnew entry
kdebrab Feb 2, 2023
c0ba45a
[CLN] minor non-functional change to improve readability
kdebrab Feb 2, 2023
ab07267
revert latest commit
kdebrab Feb 2, 2023
f295763
apply suggested changes
kdebrab Feb 2, 2023
14cd046
update docstring, add contributors
kdebrab Feb 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
[DOC] iam.physical: update docstring and add whatsnew entry
  • Loading branch information
@kdebrab
kdebrab committed Feb 2, 2023
commit f3a82dd3cd6aa43e326ae7fdde0d51bdf041a6cb
2 changes: 2 additions & 0 deletions docs/sphinx/source/whatsnew/v0.9.5.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As well as the reviewers.


Bug fixes
~~~~~~~~~
Expand Down
6 changes: 4 additions & 2 deletions pvlib/iam.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,12 @@ def ashrae(aoi, b=0.05):
def physical(aoi, n=1.526, K=4.0, L=0.002, *, n_ar=None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

r"""
Determine the incidence angle modifier using refractive index ``n``,
extinction coefficient ``K``, and glazing thickness ``L``.
extinction coefficient ``K``, glazing thickness ``L`` and refractive
index ``n_ar`` of an eventual anti-reflective coating.

``iam.physical`` calculates the incidence angle modifier as described in
[1]_, Section 3. The calculation is based on a physical model of absorption
[1]_, Section 3, with additional support of an anti-reflective coating.
The calculation is based on a physical model of reflections, absorption,
and transmission through a transparent cover.

Parameters
Expand Down
0