-
Notifications
You must be signed in to change notification settings - Fork 1.1k
add functionality to load Solcast API data to iotools
#1875
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
@lorenzo-solcast thank you for this PR! Note, we have not yet taken an official stance on iotools functions for commercial data providers so we can't promise this will get merged. Personally I'm in favor, under the condition that you take on the maintenance of the associated functions. The irradiance iotools functions follow a pattern of returning a tuple of (data, meta). If there's no metadata to parse then just return an empty dictionary. Perhaps down the road metadata will be returned, and even if not then it's offered to have a standardized format. Mock testing is probably sufficient (there's other iotools that only have that). Maybe it's even preferred. |
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.
An initial review from the airport😄
pvlib/iotools/solcast.py
Outdated
map_variables: bool, default: True | ||
When true, renames columns of the DataFrame to pvlib variable names | ||
where applicable. See variable :const:`VARIABLE_MAP`. | ||
Time is made the index with the "period mid" convention from Solcast's "period end". |
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.
Does this only happen when map_variables
is True?
I think it would be preferable to always have the same time index convention, so I would move this comment to the output section.
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.
yes, it is applied only if map_variables
is True. The idea being that the raw data is returned if set to False
, but we can do that by default if preferred?
thanks for the review @AdamRJensen! should have addressed most of your points and returning an empty
Well aware and totally up to the team. We'd be happy to contribute and take on the maintenance of the module. |
hi! any other thoughts on this PR? |
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.
Thanks @lorenzo-solcast
@pvlib/pvlib-maintainer I support adding this.
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
# Conflicts: # pvlib/iotools/solcast.py
thanks for the review @cwhanse - your suggestion makes the docstring much clearer. |
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 more comments. Thanks for the PR @lorenzo-solcast!
@lorenzo-solcast If you're able to address these issues within the week, we should be able to get these functions into the December pvlib release 🥳 |
* addressing feedback from Kandersolar
thanks for the great review @kandersolar! should have addressed all the points you raised - let me know what you think. |
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.
Solid code! Only minor stuff from me.
Note, I wasn't able to test examples of the forecast/live data though with my API key.
Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
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 we should be good to merge this function later today 🥳
Great work @lorenzo-solcast!
I would also recommend adding a "See Also" section to all the functions pointing to their Solcast cousins. Example:
|
@lorenzo-solcast I made some minor changes so don't forget to pull from GH I see that there are two lines that are not being hit by the tests, could you make some basic tests to hit these? |
@AdamRJensen added the tests! |
@lorenzo-solcast Access to Solcast data will now be supported by pvlib within a matter of days with the release of v0.10.3. Very exciting stuff! Thanks for your contribution and patience during the review process 🥳 |
exciting indeed! looking forward to contribute further and thank you for the thorough reviews. |
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.With this PR we are proposing to add a module
iotools.solcast
with functions to get irradiation and weather data for live, forecast, historic and tmy from the Solcast API. We noticed the demand for such functionality and many of our users use PVLib with Solcast data.Note that Solcast has a free tier for radiation and weather data which you can access immediately by using a company email address when signing up. We also have unlimited requests at unmetered locations and additional free data is also available for researchers and students.
This is a first iteration and are looking for feedback, in particular about the following design decisions:
iotools
modules, we have created a mapping to translate variables form Solcast's definition to PVLibs that is applied whenmap_variables
isTrue
. This is a 1-way translation so the user would still query the Solcast API with Solcast's definitions and get the variables in apd.DataFrame
with the variables converted to PVLib's. We could make the interface only using PVLib's definitions but that may cause confusion with the API documentation.mocks
. If you prefer we can provide an API key to test directly against the API.kwargs
. The intention is to avoid docstrings that are too verbose.Some tests have been added but we'll add more after a first review.
Thanks!