-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix omission in snow_coverage_nrel #2292
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
Changes from 6 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
3594450
add snow_depth inputs
cwhanse 9716bc8
linter
cwhanse cd579f2
double ticks
cwhanse c11aee4
Merge branch 'main' of https://github.com/pvlib/pvlib-python into sno…
cwhanse 7194e3c
review comments, improve some descriptions
cwhanse 11cf12e
whatsnew
cwhanse b6424e3
clarify threshold_depth
cwhanse d46837c
add note about default value
cwhanse 0ef3d7a
Merge branch 'main' of https://github.com/pvlib/pvlib-python into sno…
cwhanse 57f5eef
move check for snow depth
cwhanse 81c44d7
comments
cwhanse 4e4350e
Merge branch 'main' of https://github.com/pvlib/pvlib-python into sno…
cwhanse fb97cd9
update whatsnew
cwhanse e3d97e5
edit whatsnew
cwhanse 1113823
Merge branch 'main' into snow_depth
cwhanse 4d0110a
Apply suggestions from code review
cwhanse b1ff01a
edits
cwhanse File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this check can be applied at the end like this. In cases where the check takes effect (snow_depth = 0 --> coverage = 0), that effect should propagate forward in time and influence values for future timestamps. As it is now, I don't think this code allows that.
See for example this input:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are correct. The SAM implementation appears to iterate over timesteps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the SAM algorithm, these lines set coverage to full if both of two conditions are met:
So change in snow depth exceeds deltaThreshold (*dt converts to hourly), and snowDepth exceeds a different threshold so the snow is "sticking" around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kandersolar I think I've corrected the implementation of the check