-
Notifications
You must be signed in to change notification settings - Fork 203
Add UI elements and dialog for snow depth data downloading, access, and editing #2089
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
Conversation
… a snow data array
…f user wants to use the weather file snow data instead
…this to obscure meaningful changes to defaults, so I ran these files through the parser without making any changes. All diffs should be due to rounding.
|
The recent commits fix an important issue. The original code did not handle time zones correctly. Now, for any station, the time series will start at t=0 local time for that station. |
|
@berg-michael The |
|
@cpaulgilman sorry for the issue, I hope to be able to take a look sometime next few days. I was adding TZ data to the file itself, so it might have gotten shuffled around in that process. I'll investigate what happened and try to get a fix in. |
|
It looks like we need a version of stations.csv with time zone data to get this to work. I'm investigating further and will follow up. |
@berg-michael I fixed this by adding a time zone column to the stations.csv file, so you're off the hook. Thanks so much for your work on this! |
|
Outstanding issues:
|
sjanzou
left a comment
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.
Need to keep defmgr updates for rapidjson Inf and NaN values
| "6par_module_length": 2.277235165721802, | ||
| "6par_module_name": "Aptos Solar Technology LLC DNA-144-BF10-530W (from CEC database)", | ||
| "6par_module_width": 1.1329528187670655, | ||
| "6par_module_width": 1.1329528187670657, |
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.
Should these be showing up here?
…dates Test results updates for failing GitHub actions
sjanzou
left a comment
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.
resolved with latest commit - will check GitHub actions
Description
This PR includes the necessary changes to accommodate new snow data functionality in SAM. In particular:
snow_arrayanduse_snow_weather_file, which respectively hold potential user entered/downloaded snow depth data, and allow the user to indicate to toggle between this data and the data present in the weather file.To review:
Build against the ssc branch of the same name.
Test looking up, downloading snow depth data.
Fixes # (issue(s))
Corresponding branches and PRs:
NREL/ssc#1335
https://github.com/NREL/SAM-private/pull/128
Unit Test Impact:
A new SSC test has been written to check the correctness of the snow model when using snow data from the builtin array (as opposed to the original weather file method).
Checklist