Skip to content
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

Add gallery example for simple irradiance adjustment for horizon shading #1849

Merged
merged 20 commits into from Sep 19, 2023

Conversation

spaneja
Copy link
Contributor

@spaneja spaneja commented Sep 7, 2023

Adding gallery example of how to use the PVGIS get_horizon_data function.

  • [ ] Closes #xxxx
  • I am familiar with the contributing guidelines
  • [ ] Tests added
  • [ ] Updates entries in docs/sphinx/source/reference for API changes.
  • Adds description and name entries in the appropriate "what's new" file in 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`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

After talking to @kandersolar on the topic, it comes to mind that the newly-added get_pvgis_horizon() function is not intuitive to people who want to adjust time-series irradiance for far-horizon shading effects. As such, this Example Gallery piece shows users how to 1) get data from the function, 2) interpolate it to time-series solar azimuth values, and 3) derate DNI and global poa during times of far horizon shading.

I would also appreciate any feedback on the code, and believe I may have a few items to touch up on this example. Thank you in advance.

Adding gallery example of how to use the PVGIS get_horizon_data function.
@spaneja spaneja changed the title pvgis gallery example pvgis gallery example for pvgis horizon data Sep 7, 2023
@kandersolar kandersolar added this to the v0.10.2 milestone Sep 8, 2023
@kandersolar
Copy link
Member

I am wondering if it would make more sense to rebrand this example as a "simple horizon adjustment" example, i.e. not emphasizing that the data came from PVGIS. The adjustment approach would work the same no matter where the horizon profile came from, which to me suggests that the data source doesn't necessarily need to be the highlight. Anticipating the addition of a more complex horizon shading model in the future, maybe the title of this one could be "Simple irradiance adjustment for horizon shading"?

A related consideration is whether making web requests in the gallery is a good idea. This will be the first as far as I am aware. The concern is that pvlib.iotools functions are the flakiest (meaning most likely to "randomly" fail) part of pvlib and I am hesitant to give them more opportunity to create spurious failures in our automated processes. One alternative to fetching the horizon profile dynamically is to hardcode a profile in the script but include a comment indicating how it could be retrieved from PVGIS. Or we could configure the script to just not get executed during the docs build. I think I lean towards hardcoding a profile but am curious what others think.

@cwhanse
Copy link
Member

cwhanse commented Sep 11, 2023

One alternative to fetching the horizon profile dynamically is to hardcode a profile in the script but include a comment indicating how it could be retrieved from PVGIS

I'm in favor of this, provided the cached file (which would go in the data folder is a reasonable size.

@kandersolar
Copy link
Member

No file necessary, I think. PVGIS horizon profiles are just 48 floats spaced evenly around the horizon, so might as well just include the values in the script itself. In this example, the call to get_pvgis_horizon could be replaced with:

horizon_profile = pd.Series([
    10.7, 11.8, 11.5, 10.3,  8. ,  6.5,  3.8,  2.3,  2.3,  2.3,  4.6,  8. ,
    10.3, 11.1, 10.7, 10.3,  9.2,  6.1,  5.3,  2.3,  3.1,  1.9,  1.9,  2.7,
     3.8,  5.3,  6.5,  8.4,  8.8,  8.4,  8.4,  8.4,  6.5,  6.1,  6.5,  6.1,
     7.3,  9.2,  8.4,  8. ,  5.7,  5.3,  5.3,  4.2,  4.2,  4.2,  7.3,  9.5
], index=np.arange(0, 360, 7.5))

@AdamRJensen
Copy link
Member

I'm in favor of rebranding it as "Simple horizon shading".

I have it on my to-do list one day to add more complicated shading where the horizon brightening is also affected by the horizon line and the sky diffuse is reduced - but that is in the somewhat distant future.

Also, I'm +0.1 in hard-coding the horizon profile as suggested by @kandersolar. If that's the case, then I recommend making an admonition with an example of how to use PVGIS that could be copy pasted instead of the hard-coded profile. For example, something like this:

💥 Retrieving horizon profile from PVGIS
horizon_profile = pvlib.iotools.get_pvgis_horizon(latitude=x, longitude=y)

Adjusted to the broader 'simple irradiance...shading' rebrand, use hard-coded horizon profile data, and give example of how to get such data from PVGIS function in IO Tools.
@spaneja
Copy link
Contributor Author

spaneja commented Sep 12, 2023

Thanks @kandersolar and @AdamRJensen. I think this makes sense, and I agree that having an example of using the IO Tools function would be helpful.

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.

Thanks @spaneja! I have a few suggestions below about the presentation but otherwise I think this is looking good.

Updated whatsnew file with pull #. Updated formatting and other minor syntax in .py file.
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. Thanks @spaneja!

Copy link
Member

@AdamRJensen AdamRJensen left a comment

Choose a reason for hiding this comment

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

@spaneja Great contribution!

spaneja and others added 6 commits September 19, 2023 08:37
Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
…rizon_shading.py

Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
…rizon_shading.py

Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
…rizon_shading.py

Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
…rizon_shading.py

Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
…rizon_shading.py

Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
spaneja and others added 3 commits September 19, 2023 08:39
…rizon_shading.py

Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
…rizon_shading.py

Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
…rizon_shading.py

Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
Copy link
Member

@AdamRJensen AdamRJensen left a comment

Choose a reason for hiding this comment

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

Looking good! Just a few minor comments.

Latitude/longitude are standard names in the pvlib world, thus I have a mild preference for not shortening them.

spaneja and others added 4 commits September 19, 2023 09:17
…rizon_shading.py

Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
…rizon_shading.py

Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
Corrected docstring text
@kandersolar kandersolar changed the title pvgis gallery example for pvgis horizon data Add gallery example for simple irradiance adjustment for horizon shading Sep 19, 2023
@kandersolar kandersolar merged commit 9b4317e into pvlib:main Sep 19, 2023
24 of 30 checks passed
@kandersolar
Copy link
Member

Thanks @spaneja, this is great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants