-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Change surface_azimuth convention in get_pvgis_hourly #1739
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
Change surface_azimuth convention in get_pvgis_hourly #1739
Conversation
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 supplied azimuth is echoed in the inputs
return value... I guess we should translate that one as well? Or maybe just get rid of that return value altogether so that the function can have the same data, metadata
return structure as the rest of iotools?
Co-authored-by: Kevin Anderson <kevin.anderso@gmail.com>
So the second returned element "inputs" actually also contains metadata describing the outputs, so it is useful. I am a strong believer in sticking with the (data, meta) output format and think that the output related content in "inputs" should simply be included in the meta dictionary and the input related content in "inputs" should be discarded. Consistency is key, and I always forget how many items the PVGIS functions return. But changing this would be a breaking change...? |
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.
Regarding inputs
: how about we translate azimuth in this PR and open a new issue for changing the number of return values? Might want to get that into 0.10.0 too.
Translating the azimuth is surprisingly convoluted. First off, it is necessary to update both the description:
and the value (changing the value requires some wrangling due to the field name changing depending on the type of mounting system):
However, the above line is not sufficient, given that for certain tracking mounting systems the azimuth value is specified as '-'. Also, the metadata structure is completely different for the json and the csv output formats. In short, translating the azimuth will add significant lines of code to the function, which I don't think is worth it. I'm more inclined to add a warning message stating that the metadata concerning the azimuth is not corrected. @kandersolar what do you think of this? |
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'm more inclined to add a warning message stating that the metadata concerning the azimuth is not corrected. @kandersolar what do you think of this?
I guess a "warning" does not refer to warnings.warn()
but rather text in the docstring? If so, ok with me!
Co-authored-by: Kevin Anderson <kevin.anderso@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.
LGTM!
pvlib.iotools.get_pvgis_hourly
'ssurface_azimuth
parameter doesn't use pvlib's azimuth convention #1724[ ] Updates entries indocs/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.Nearly everything in pvlib represents azimuth angles as values in [0, 360) clockwise from north, except
pvlib.iotools.get_pvgis_hourly
. This PR modifies the get_pvgis_hourly function such that it follows the pvlib convention.