-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix inconsistencies #1268
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
Fix inconsistencies #1268
Conversation
This reverts commit d7deb80.
Enforce the usage of the following terminology in the iotools inputs: start/end, latitude/longitude, and metadata. Also, latitude/longitude should come before start/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.
Thanks for this cleanup @AdamRJensen! Comments below, and two more here:
- We'll have to update the existing PVGIS tests at some point before 0.10, might as well specify
map_variables=False
now so that it's not emitting warnings in the mean time. introtutorial.rst
usespvlib.iotools.get_pvgis_tmy
, might as well usemap_variables=True
there too since it does the same thing manually anyway.
Thanks for the review @kanderso-nrel. Consider all changes implemented. I've added variable_map=False to all pvgis_tmy tests and they are silent now. I'm beginning to see something beautiful about the code review process - it truly does make for much better and consistent code. |
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.
LGTM, would be good to get someone else's (@wholmgren?) feedback too because breaking changes are always scary.
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 @AdamRJensen. Check out the code below for an example of using fail_on_pvlib_version
pvlib-python/pvlib/tests/test_modelchain.py
Lines 1781 to 1788 in 0fd5c43
@fail_on_pvlib_version('1.0') | |
def test_ModelChain_attributes_deprecated_10(sapm_dc_snl_ac_system, location): | |
match = 'Use ModelChain.results' | |
mc = ModelChain(sapm_dc_snl_ac_system, location) | |
with pytest.warns(pvlibDeprecationWarning, match=match): | |
mc.aoi | |
with pytest.warns(pvlibDeprecationWarning, match=match): | |
mc.aoi = 5 |
While you're at it, can you change that decorator argument to '0.10'
?
return data | ||
|
||
if map_variables is None: | ||
warnings.warn( |
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.
We need to test that this warning is emitted and mark that test with the fail_on_pvlib_version
decorator. This ensures that we don't forget to remove deprecated functionality.
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.
@wholmgren Very smart, thanks for showing me this!
I believe all of your comments have been addressed.
Co-authored-by: Will Holmgren <william.holmgren@gmail.com>
Co-authored-by: Will Holmgren <william.holmgren@gmail.com>
Co-authored-by: Will Holmgren <william.holmgren@gmail.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.
@kanderso-nrel ready to merge?
Enforce the usage of the following terminology in pvlib iotools inputs: start/end, latitude/longitude, and metadata. Also, location information (such as latitude/longitude or station name) should come before start/end.
Updates entries todocs/sphinx/source/api.rst
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`
).