-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add NASA POWER to iotools #2500
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
base: main
Are you sure you want to change the base?
Conversation
@AdamRJensen could you give some first feedback on this? Also, should there also be a |
I suggest just adding a |
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.
Initial review
Co-authored-by: Adam R. Jensen <[email protected]>
Co-authored-by: Adam R. Jensen <[email protected]>
Co-authored-by: Adam R. Jensen <[email protected]>
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.
A few comments, overall looks pretty good to me! Seems like the API is a little slow but not terrible: 16 seconds for a year of data.
community: str | ||
Can be one of the following depending on which parameters are of | ||
interest. The default is ``'re'``. Note that in many cases this choice |
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.
community: str | |
Can be one of the following depending on which parameters are of | |
interest. The default is ``'re'``. Note that in many cases this choice | |
community: str, default 're' | |
Can be one of the following depending on which parameters are of | |
interest. Note that in many cases this choice |
map_variables: bool, optional | ||
When true, renames columns of the Dataframe to pvlib variable names | ||
where applicable. See variable :const:`VARIABLE_MAP`. | ||
The default is `True`. |
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.
map_variables: bool, optional | |
When true, renames columns of the Dataframe to pvlib variable names | |
where applicable. See variable :const:`VARIABLE_MAP`. | |
The default is `True`. | |
map_variables: bool, default True | |
When true, renames columns of the Dataframe to pvlib variable names | |
where applicable. See variable :const:`VARIABLE_MAP`. |
|
||
def test_get_nasa_power(data_index, ghi_series): |
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.
def test_get_nasa_power(data_index, ghi_series): | |
@pytest.mark.remote_data | |
@pytest.mark.flaky(reruns=RERUNS, reruns_delay=RERUNS_DELAY) | |
def test_get_nasa_power(data_index, ghi_series): |
All test functions that access internet APIs should have these two decorators :)
And add from tests.conftest import RERUNS, RERUNS_DELAY
at the top of the file.
* ``ALLSKY_SFC_SW_DIFF``: Diffuse Horizontal Irradiance (DHI) [Wm⁻²] | ||
* ``ALLSKY_SFC_SW_DNI``: Direct Normal Irradiance (DNI) [Wm⁻²] | ||
* ``T2M``: Air temperature at 2 m [C] | ||
* ``WS10M``: Wind speed at 10 m [m/s] |
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 suggest using pvlib names here and in the default parameter list. As a user, I don't want to have to mentally translate ALLSKY_SFC_SW_DWN
into ghi
when reading these docs.
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 to clarify: are you suggesting that the pvlib function accept values like 'ghi'
in place of 'ALLSKY_SFC_SW_DIFF'
?
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.
The pvlib function currently accepts both of those and I am not suggesting changing that. The suggestion is for the default values to use pvlib names instead of NASA POWER names, to (IMHO) make the docstring a little clearer.
hourly_data = data['properties']['parameter'] | ||
df = pd.DataFrame(hourly_data) | ||
df.index = pd.to_datetime(df.index, format='%Y%m%d%H').tz_localize('UTC') | ||
df = df.replace(-999, np.nan) |
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.
perhaps worth using meta['fill_value']
instead of hardcoding -999, in case they change it someday?
docs/sphinx/source/reference
for API changes.docs/sphinx/source/whatsnew
for 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.This PR adds a function to get irradiance and weather data from NASA POWER. read_the_docs_link