Skip to content
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

Should marion_diffuse use diffuse versions of martin_ruiz and schlick? #2113

Closed
markcampanelli opened this issue Jun 30, 2024 · 4 comments
Closed

Comments

@markcampanelli
Copy link
Contributor

Describe the bug

While working on #2050, it occurred to me that pvlib.iam.marion_diffuse refers to the direct component IAM functions for martin_ruiz and schlick, i.e., it would seem that these two lines should instead refer to pvlib.iam.martin_ruiz_diffuse and pvlib.iam.schlick_diffuse

To Reproduce
Steps to reproduce the behavior:

  1. Inspect the lines at: https://github.com/pvlib/pvlib-python/blob/main/pvlib/iam.py#L630-L631

Expected behavior
I would expect the diffuse IAM models to be used, because marion_diffuse computes diffuse components.

Screenshots
None

Versions:

  • pvlib.__version__: v0.11,0
  • python: 3.11.7

Additional context
If someone could verify, then I would be happy to fix this as part of #2050.

@echedey-ls
Copy link
Contributor

If I understand correctly, that diffuse IAM uses direct IAM models to compute a diffuse one via integration. So it sounds good to me. It uses this function: https://github.com/pvlib/pvlib-python/blob/13ef8f4e3f07a6b1c5acd7014a2617beeb10867c/pvlib/iam.py#L647C1-L653C34

@markcampanelli
Copy link
Contributor Author

If I understand correctly, that diffuse IAM uses direct IAM models to compute a diffuse one via integration. So it sounds good to me. It uses this function: https://github.com/pvlib/pvlib-python/blob/13ef8f4e3f07a6b1c5acd7014a2617beeb10867c/pvlib/iam.py#L647C1-L653C34

I think I understand now: pvlib.iam.martin_ruiz_diffuse and pvlib.iam.schlick_diffuse are analogous to pvlib.iam.marion_diffuse, but with a slightly different output interface. I was wondering why one would need a separate IAM function for direct vs. diffuse 🤪. Thanks @echedey-ls!

@markcampanelli
Copy link
Contributor Author

Closing due to misunderstanding of code. (Github's issue-closing interface is a bit wierd!)

@markcampanelli
Copy link
Contributor Author

Really, close this.

@markcampanelli markcampanelli closed this as not planned Won't fix, can't repro, duplicate, stale Jun 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants