Add retrieval functions for daily precip, snow, and temperature data from NOAA RCC ACIS #1767
Conversation
kandersolar
added
remote-data
kandersolar
added
remote-data
|
Test failures are something to do with forecasts which I'm not excited about debugging given #1766. |
|
After further consideration, I think there are enough differences between datasets that it makes sense to rewrite this PR so that there is a 1:1 correspondence between |
kandersolar
added
remote-data
kandersolar
changed the title
|
@AdamRJensen this is ready for another look. In addition to splitting up the function as mentioned above, I also added functions for the weather station dataset. A discussion point: currently, Test failures are |
kandersolar
changed the title
| # time series names | ||
| 'pcpn': 'precipitation', | ||
| 'maxt': 'temp_air_max', | ||
| 'avgt': 'temp_air_average', |
There was a problem hiding this comment.
Perhaps remove _average if this is almost always the temperature of interest for PV applications? For example, the bsrn function doesn't append _average but does append _min and _max.
There was a problem hiding this comment.
I lean towards keeping the _average suffix here. Daily values seem different enough from the higher-resolution values that we call temp_air that consistency with the other names in this dict seems more important to me. It doesn't make sense to feed these values into ModelChain.run_model, for example.
Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
There was a problem hiding this comment.
Thanks @AdamRJensen!
My main concern is that it is a lot of functions (clutter) compared to the previous state of the PR which had it combined into one function.
Yes, it does add a bit of clutter to the API listing. But there are enough differences between the datasets that I thought splitting it up would make it easier to use and more straightforward to maintain, which seemed more important.
Any thoughts on this question from #1767 (comment)?
A discussion point: currently, get_acis_station_data returns precipitation in mm but snow in cm, for consistency with relevant PV models (pvlib.soiling takes rainfall in mm; pvlib.snow takes snowfall in cm). Is that the right choice?
| # time series names | ||
| 'pcpn': 'precipitation', | ||
| 'maxt': 'temp_air_max', | ||
| 'avgt': 'temp_air_average', |
There was a problem hiding this comment.
I lean towards keeping the _average suffix here. Daily values seem different enough from the higher-resolution values that we call temp_air that consistency with the other names in this dict seems more important to me. It doesn't make sense to feed these values into ModelChain.run_model, for example.
There was a problem hiding this comment.
thanks @kandersolar - looking forward to using this!
As for snow units, I think I'd rather keep them in mm here and make other functions consistent with that. But ask me again tomorrow and I might have a different opinion.
kandersolar
added
remote-data
I think I agree with Will - we should avoid having too many different units. |
kandersolar
added
remote-data
|
See a bit of discussion in #1792 about snow units. Keeping them as cm seems like the best option to me, at least for now. I think that was the last open item here, so in it goes! |
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.ACIS provides precipitation estimates from PRISM, plus three other gridded datasets. No registration needed.
ACIS has another API endpoint for station-recorded (not gridded) data that supplies snowfall, among other variables.
Possible future PR for that.now included in this PR