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

Implement C1 continuous version of the Erbs diffuse-fraction/decomposition model #1834

Merged
merged 8 commits into from Aug 26, 2023

Conversation

adriesse
Copy link
Member

@adriesse adriesse commented Aug 14, 2023

@adriesse
Copy link
Member Author

adriesse commented Aug 14, 2023

This new model/function is using the same structure as all the other diffuse-fraction/decomposition functions with one exception: it has an optional input for dni_extra, which makes datetime_or_doy also optional.

docs/sphinx/source/whatsnew/v0.10.2.rst Outdated Show resolved Hide resolved
pvlib/irradiance.py Outdated Show resolved Hide resolved
pvlib/irradiance.py Show resolved Hide resolved
@cwhanse cwhanse added this to the v0.10.2 milestone Aug 14, 2023
adriesse and others added 2 commits August 15, 2023 10:03
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
pvlib/irradiance.py Outdated Show resolved Hide resolved
pvlib/irradiance.py Show resolved Hide resolved
@adriesse
Copy link
Member Author

@kandersolar you have a very good point that I have also thought about. Here are a two options that are not mutually exclusive:

  • delay the merge and reconsider the question just before the next release
  • give the PR reviewers access to our manuscript

Here is a plot from the manuscript:

erbs_discontinuity

@cwhanse
Copy link
Member

cwhanse commented Aug 16, 2023

@kandersolar we added the sandia_multi inverter function before the reference paper was published. The extension of iam.physical to handle multiple interfaces was originated in pvlib discussions, if I'm not mistaken. So we've made exceptions before.

Copy link
Member

@wholmgren wholmgren left a comment

Choose a reason for hiding this comment

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

Thanks @adriesse!

I don't think we need to wait for a publication in this case.

pvlib/irradiance.py Outdated Show resolved Hide resolved
@adriesse
Copy link
Member Author

Does anyone have an idea why these test failures started to pop up?

@kandersolar
Copy link
Member

I suspect RuntimeError: Optimal parameters not found: The maximum number of function evaluations is exceeded. errors are due to scipy/scipy#19103.

Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

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

Thanks @adriesse!

@adriesse
Copy link
Member Author

Ok, seems ready to merge then--or do we have to wait for scipy to sort itself out?

@kandersolar kandersolar merged commit 0c66e09 into pvlib:main Aug 26, 2023
18 of 25 checks passed
@kandersolar
Copy link
Member

do we have to wait for scipy to sort itself out?

Nope, no reason to hold this up for that :)

@adriesse
Copy link
Member Author

Thanks @ all!

@adriesse adriesse deleted the decomp branch August 26, 2023 16:01
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.

Erbs diffuse fraction model has discontinuities at transition points
4 participants