Implemented closed-form solution in detect_clearsky
#2217
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 3 commits
September 19, 2024 15:50
…learsky. Updates test." This reverts commit 94259c5.
echedey-ls
reviewed
Sep 19, 2024
There was a problem hiding this comment.
Nice PR @agodbehere 💯 !!
Leaving early review comments:
- You can safely address flake8 warnings in
test_clearsky.py - Docs will need to be updated to reflect what new algorithm is being used, which I identify are:
- The docstring of the modified function (
pvilb.clearsky.detect_clearsky) - The user guide section at
clearsky.rst - As you update these sections, you will be able to see how they will look when published at the following link (you can access it at the bottom of this PR in "Checks", through the "ReadTheDocs" action): https://pvlib-python--2217.org.readthedocs.build/en/2217/
- The docstring of the modified function (
- I haven't checked the code much more deeply, but there may be parts relating to the old algorithm at other places too
Feel free to ask about whatever you need help with, we all know it's not that easy to contribute to open-source 🚀
| @@ -30,6 +30,10 @@ Enhancements | |||
| * Added function for calculating wind speed at different heights, | |||
| :py:func:`pvlib.atmosphere.windspeed_powerlaw`. | |||
| (:issue:`2118`, :pull:`2124`) | |||
| * Implemented closed-form solution for alpha in detect_clearsky, obviating | |||
| the call to scipy.optimize that was prone to runtime errors and minimizing | |||
| computation, :py:func:`pvlib.clearsky.detect_clearsky`. Resolves :issue:`2712`. | |||
There was a problem hiding this comment.
Suggested change
| computation, :py:func:`pvlib.clearsky.detect_clearsky`. Resolves :issue:`2712`. | |
| computation, :py:func:`pvlib.clearsky.detect_clearsky`. (:discuss:`2171`, :issue:`2216`, :pull:`2217`) |
There was a problem hiding this comment.
Should be :issue:2171, :issue:2216
|
|
||
| # Compute arg min of MSE between model and observations | ||
| C = (clear_clear**2).sum() | ||
| if not (pd.isna(C) or C == 0): # safety check |
There was a problem hiding this comment.
What would physically mean that this value is zero or NaN? Maybe we can skip this safety check, I wonder what could happen if this code isn't executed
There was a problem hiding this comment.
Without the check the next line divides by C. The exit criteria need alpha to be a float. I think the check and if...break are fine.
RDaxini
reviewed
Sep 20, 2024
Comment on lines
33
to
35
| * Implemented closed-form solution for alpha in detect_clearsky, obviating | ||
| the call to scipy.optimize that was prone to runtime errors and minimizing | ||
| computation, :py:func:`pvlib.clearsky.detect_clearsky`. Resolves :issue:`2712`. |
There was a problem hiding this comment.
Link the function in the first line and then no need to repeat it here at the end as (I think?) we only do that if it is a new function being added
Suggested change
| * Implemented closed-form solution for alpha in detect_clearsky, obviating | |
| the call to scipy.optimize that was prone to runtime errors and minimizing | |
| computation, :py:func:`pvlib.clearsky.detect_clearsky`. Resolves :issue:`2712`. | |
| * Implemented closed-form solution for alpha in | |
| :py:func:`pvlib.clearsky.detect_clearsky`, obviating | |
| the call to scipy.optimize that was prone to runtime errors and minimizing | |
| computation. (:issue:`2171`, :issue:`2216`, :pull:`2217`). |
cwhanse
approved these changes
Sep 23, 2024
RDaxini
approved these changes
Sep 23, 2024
kandersolar
changed the title
detect_clearsky
|
Thanks for this nice improvement @agodbehere! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
docs/sphinx/source/referencefor API changes.docs/sphinx/source/whatsnewfor 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`).remote-data) and Milestone are assigned to the Pull Request and linked Issue.