Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Merged
merged 14 commits into from
May 25, 2023

Conversation

AdamRJensen
Copy link
Member

@AdamRJensen AdamRJensen commented May 16, 2023

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.

@AdamRJensen AdamRJensen added the io label May 16, 2023
@AdamRJensen AdamRJensen added this to the 0.10.0 milestone May 16, 2023
Copy link
Member

@kandersolar kandersolar left a 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?

pvlib/iotools/pvgis.py Show resolved Hide resolved
docs/sphinx/source/whatsnew/v0.9.6.rst Outdated Show resolved Hide resolved
pvlib/iotools/pvgis.py Outdated Show resolved Hide resolved
@AdamRJensen
Copy link
Member Author

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?

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...?

Copy link
Member

@kandersolar kandersolar left a 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.

pvlib/iotools/pvgis.py Show resolved Hide resolved
@AdamRJensen AdamRJensen added the remote-data triggers --remote-data pytests label May 22, 2023
@AdamRJensen
Copy link
Member Author

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:

metadata['inputs']['mounting_system']['fields']['azimuth']['description'] = \
    'Orientation (azimuth) angle of the (fixed) PV system (0 = N, 90 = E, 180 = S)'

and the value (changing the value requires some wrangling due to the field name changing depending on the type of mounting system):

inputs['mounting_system'][list(inputs['mounting_system'].keys())[0]]['azimuth']['value'] = \
    inputs['mounting_system'][list(inputs['mounting_system'].keys())[0]]['azimuth']['value'] + 180

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?

Copy link
Member

@kandersolar kandersolar left a 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!

pvlib/iotools/pvgis.py Outdated Show resolved Hide resolved
Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@kandersolar kandersolar merged commit 5119b42 into pvlib:main May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api io remote-data triggers --remote-data pytests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pvlib.iotools.get_pvgis_hourly's surface_azimuth parameter doesn't use pvlib's azimuth convention
2 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.