Adding altitude lookup for Location class #1850
Conversation
Set altitude automatically from built-in map when not specified. See pvlib#1547
| @@ -22,6 +22,8 @@ Enhancements | |||
| * Added :py:func:`~pvlib.iam.interp` option as AOI losses model in | |||
| :py:class:`pvlib.modelchain.ModelChain` and | |||
| :py:class:`pvlib.pvsystem.PVSystem`. (:issue:`1742`, :pull:`1832`) | |||
| * Default altitude in :py:class:`pvlib.location.Location` | |||
There was a problem hiding this comment.
Is this considered a breaking change?
The api is technically the same, but it can cause issues downstream
There was a problem hiding this comment.
It is, if the user has been omitting altitude for locations where there is data. I'm willing to live with it because the effect on air mass and other downstream variables is small.
If we want to deprecate, we could hold this PR for 10.3 and emit a warning of the future change in v10.2 instead. @kandersolar ?
There was a problem hiding this comment.
I think what is most consistent with our versioning strategy is to wait until 0.11.0 for this (unless we consider it a bugfix, which is a stretch IMHO).
Either way I wouldn't bother with a deprecation.
There was a problem hiding this comment.
Hi @kandersolar, Sounds good!
If you switch the milestone to 0.11.0 would I get a notification when it gets a release date?
I can resolve conflicts then
There was a problem hiding this comment.
Unfortunately I'm not aware of a way for you to automatically get notified when we start working on 0.11.0. But anyway we will see this PR in the list and come back to it then. Thanks for the contribution :)
There was a problem hiding this comment.
Hi @nicomt, we're working towards v0.11.0 now, so the time has come to dust off the PR if you're still interested :)
| @@ -22,6 +22,8 @@ Enhancements | |||
| * Added :py:func:`~pvlib.iam.interp` option as AOI losses model in | |||
| :py:class:`pvlib.modelchain.ModelChain` and | |||
| :py:class:`pvlib.pvsystem.PVSystem`. (:issue:`1742`, :pull:`1832`) | |||
| * Default altitude in :py:class:`pvlib.location.Location` | |||
There was a problem hiding this comment.
I think what is most consistent with our versioning strategy is to wait until 0.11.0 for this (unless we consider it a bugfix, which is a stretch IMHO).
Either way I wouldn't bother with a deprecation.
Better type definition Co-authored-by: Kevin Anderson <[email protected]>
|
Thanks @nicomt! |
|
Thanks for finishing the PR @kandersolar. I tried to look at this sooner but had a busy week. I'm glad is in now 😄 |
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.Set altitude automatically from built-in map when not specified for the Location class.
See #1547