Skip to content

Implement iam.schlick, iam.schlick_diffuse #1562

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.

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 32 commits into from
Nov 28, 2022

Conversation

kandersolar
Copy link
Member

@kandersolar kandersolar commented Sep 30, 2022

  • Closes Add iam function based on Schlick 1994 #1564
  • 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.

Yu Xie from NREL presented a new IAM model at the recent PVPMC meeting (slides) and wanted to contribute it to pvlib. This one is a little unusual in that it calculates the IAM factor as a relative quantity, taking the indices of refraction of both the PV surface and the accompanying pyranometer into account.

@kandersolar kandersolar added this to the 0.9.4 milestone Sep 30, 2022
@kandersolar
Copy link
Member Author

I suppose we could move the diffuse calculations to a separate function like was done with martin_ruiz. It's a little strange to return scalar sky & ground IAM but time series direct IAM as the current code does for a fixed tilt simulation.

@kandersolar kandersolar marked this pull request as draft October 6, 2022 20:17
@kandersolar kandersolar changed the title Implement iam.fedis Implement iam.fedis and iam.schlick Oct 10, 2022
@kandersolar
Copy link
Member Author

Offline discussion with @cwhanse and the model's author determined that the parameter this PR calls n_ref may have some niche uses but should be set to the same value as n for standard PV modeling workflows (unlike as assumed in the reference). That is now the default behavior.

I also added pvlib.iam.schlick in this PR as suggested in #1564.

@kandersolar kandersolar marked this pull request as ready for review October 10, 2022 17:34
Copy link
Member

@cwhanse cwhanse left a comment

Choose a reason for hiding this comment

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

I think there are some minor efficiency improvements that could be done in iam.fedis at the cost of less transparency when comparing to the reference. E.g., the default case n_ref=n means that term1 is identically 1. On the whole I am preferring transparency.

@adriesse
Copy link
Member

Should/does fedis direct be equal to schlick? If not, why not?

@kandersolar
Copy link
Member Author

Should/does fedis direct be equal to schlick? If not, why not?

Close-ish but not identical. FEDIS is using the true Fresnel equations for the direct component so it cannot match exactly. The Schlick approximation only comes in for the diffuse components: in the absence of an analytical integration of the real Fresnel equations, FEDIS's diffuse components are instead an integration of the Shlick approximation. It's the alternative to Marion's integration method -- whereas Marion's is an approximate integration of an exact integrand, FEDIS is an exact integration of an approximate integrand.

@adriesse
Copy link
Member

Close-ish but not identical. FEDIS is using the true Fresnel equations for the direct component

I see, but we already have that in iam.physical, right?

@kandersolar
Copy link
Member Author

I see, but we already have that in iam.physical, right?

Yes, with two minor differences: FEDIS does not consider extinction (i.e. it assumes K=0 in iam.physical), and FEDIS has the option to normalize the IAM profile with respect to another device with a different index of refraction. The latter is what prevents iam.physical from being a superset of FEDIS's direct IAM calculation. A separate index of refraction for the normal incidence normalization coefficient is probably only useful in unusual modeling scenarios, but nevertheless it is part of the model so I include it here.

For context, I have the sense that the FEDIS paper to some extent views its diffuse IAM equations as the primary scientific contribution with the direct component being included for completeness.

Co-Authored-By: Anton Driesse <[email protected]>
Copy link
Member

@cwhanse cwhanse left a comment

Choose a reason for hiding this comment

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

A little docstring tidying. OK with me to merge as is.

@adriesse
Copy link
Member

Perhaps we should invite a additional reviewer on this one?

@adriesse
Copy link
Member

It turns out that term1 in fedis_diffuse() is exactly the same as the weighting function w in fedis(), just in simplified form. I suggest making the code the same in both functions and referring to this ratio/weight/multiplier as w0. (Or the calculation could even be put in a separate helper function.)

For clarity, I would suggest using the long version because it shows what's actually going on, and putting the short version in a comment with reference to the paper.

@kandersolar
Copy link
Member Author

For clarity, I would suggest using the long version because it shows what's actually going on, and putting the short version in a comment with reference to the paper.

I have kept the equations as-is so they continue being directly tied to the reference, but the latest commit at least leaves a comment noting the equivalence for any interested readers. Hopefully that achieves the same goal?

Perhaps we should invite a additional reviewer on this one?

I wouldn't mind input from additional reviewers here, but the three of us (@adriesse, @cwhanse, myself) reaching consensus would be good enough for me.

@kandersolar kandersolar changed the title Implement iam.fedis and iam.schlick Implement iam.fedis, fedis_diffuse, schlick, schlick_diffuse Oct 31, 2022
@kandersolar kandersolar changed the title Implement iam.fedis, fedis_diffuse, schlick, schlick_diffuse Implement iam.schlick, iam.schlick_diffuse Nov 17, 2022
@kandersolar
Copy link
Member Author

Following offline discussion, we have decided to move forward here with only the two schlick functions. I'll wait a few days to merge this to leave opportunity to review for anyone who wants to.

@kandersolar kandersolar merged commit 8e0b724 into pvlib:main Nov 28, 2022
@kandersolar kandersolar deleted the iam.fedis branch November 28, 2022 17:58
@kandersolar
Copy link
Member Author

Thanks again for the thoughtful and productive discussion @adriesse and @cwhanse

@adriesse
Copy link
Member

No problem. I learned a few things in the process!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add iam function based on Schlick 1994
3 participants