Skip to content

Conversation

@ericf900
Copy link
Contributor

@ericf900 ericf900 commented Mar 11, 2020

  • I am familiar with the contributing guidelines
  • 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 and Milestone are assigned to the Pull Request and linked Issue.

Example showing transposition from GHi to POA, and comparing winter transposition gain to summer.

@kandersolar
Copy link
Member

Hey @ericf900, cool example! This PR's version of the docs are built here, so you can check out how your example looks when it's live (the build takes a few minutes after you push an update): https://pvlib-python--933.org.readthedocs.build/en/933/

A couple notes:

  • The docs build system only build files whose names start with "plot_", that's why your example isn't showing up like the others. If you rename the file and then push that commit, the docs will get rebuilt.
  • Typically we strip out the spyder header (# -*- coding: utf-8 -*- / Created on... etc) before merging into pvlib
  • You should add an entry about this example under the Documentation header and (if you're comfortable with it) your name to the Contributors section in the version 0.7.2 whatsnew file: https://github.com/pvlib/pvlib-python/blob/master/docs/sphinx/source/whatsnew/v0.7.2.rst
@kandersolar
Copy link
Member

Also can you merge upstream/master into your branch? Some fixes were recently made to clean up the documentation build log and it would make reviewing this a little easier.

@ericf900
Copy link
Contributor Author

@kanderso-nrel Thanks for the suggestions! I made those updates and updated the PR

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.

Here are some suggestions. The :py:meth:... stuff will auto-link the functions to their documentation entries (see here for an example of what it looks like). Otherwise looks good to me!


# For this example, we will be using Golden, Colorado
tz = 'MST'
lat, lon = 39.755, -105.221
Copy link
Member

Choose a reason for hiding this comment

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

very nice choice of location, +1

dni=clearsky_ghi['dni'],
ghi=clearsky_ghi['ghi'],
dhi=clearsky_ghi['dhi'],
solar_zenith=solar_position['zenith'],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
solar_zenith=solar_position['zenith'],
solar_zenith=solar_position['apparent_zenith'],

FYI I'd like someone else to confirm that using apparent solar position is appropriate before making this change

Copy link
Member

Choose a reason for hiding this comment

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

According to the parameters for each transposition model option in get_total_irradiance, apparent_zenith should be passed through, rather than zenith.

It's a different matter to verify that each model actually expects apparent_zenith rather than zenith. I'll look at the underlying references when I can find the opportunity.

ericf900 and others added 9 commits March 11, 2020 09:27
Co-Authored-By: Kevin Anderson <57452607+kanderso-nrel@users.noreply.github.com>
Co-Authored-By: Kevin Anderson <57452607+kanderso-nrel@users.noreply.github.com>
Co-Authored-By: Kevin Anderson <57452607+kanderso-nrel@users.noreply.github.com>
Co-Authored-By: Kevin Anderson <57452607+kanderso-nrel@users.noreply.github.com>
Co-Authored-By: Kevin Anderson <57452607+kanderso-nrel@users.noreply.github.com>
winter_irradiance = get_irradiance(site, '12-21-2020', 25, 180)

# Plot GHI vs. POA for winter and summer
fig, (ax1, ax2) = plt.subplots(1, 2)
Copy link
Member

@mikofski mikofski Mar 11, 2020

Choose a reason for hiding this comment

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

This example is really great! Thanks!

I have a minor suggestion, can you vertically stack the plots and share the x-axis?

fig, ax = plt.subplots(2, 1, sharex=True) # stack plots (2, 1) and share the x axis ax1, ax2 = ax # or you can just use ax[0] instead of ax1, and ax[1] instead ax2, minor preference

I'm having a hard time reading the dates on the x-axis because they're a bit crowded, so I thought trying them vertical and sharing them might look nicer, but your call - but not a blocker for me - also fine as is, THANKS!

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately the indexes are 6 months apart so I don't think sharex=True will look good. Could convert the datetime index to a nicer string form with df.index.strftime("%H:%M"). Or could drop sharex=True, but then the x-label for the upper axes might overlap the bottom axes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Mike and Kevin! I am looking at this now- I agree the x-axis is a bit cluttered. I think the benefit of keeping the plots side-by-side is that it highlights that, while there is not much of a gain for POA compared to GHI in the summer, overall irradiance is higher.

I'll make some edits now and push up the new version!

Copy link
Member

Choose a reason for hiding this comment

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

You're both right! Sorry for the distraction! Great work

Copy link
Member

@cwhanse cwhanse left a comment

Choose a reason for hiding this comment

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

Thanks @ericf900

plt.show()

# %%
# Note that in Summer, there is not much gain when comparing POA irradiance to
Copy link
Member

Choose a reason for hiding this comment

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

+1 for this note, answers a question often asked by newcomers to PV modeling

dni=clearsky_ghi['dni'],
ghi=clearsky_ghi['ghi'],
dhi=clearsky_ghi['dhi'],
solar_zenith=solar_position['zenith'],
Copy link
Member

Choose a reason for hiding this comment

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

According to the parameters for each transposition model option in get_total_irradiance, apparent_zenith should be passed through, rather than zenith.

It's a different matter to verify that each model actually expects apparent_zenith rather than zenith. I'll look at the underlying references when I can find the opportunity.

@wholmgren wholmgren added this to the 0.7.2 milestone Mar 11, 2020
Copy link
Member

@mikofski mikofski left a comment

Choose a reason for hiding this comment

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

Eric, this is a really great contribution! Thanks so much. I really like the comments, super thorough and easy to read. Very good explanation of the process of generating clear-sky and transposing to POA. Awesome!

winter_irradiance['POA'].plot(ax=ax2, label='POA')
ax1.set_xlabel('Time of day (Summer)')
ax2.set_xlabel('Time of day (Winter)')
ax1.set_ylabel('Irradiance (W/m2)')
Copy link
Member

Choose a reason for hiding this comment

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

you can use latex here for W/m^2 if you want, it might look a little bit nicer?

ax1.set_ylabel('Irradiance ($W/m^2$)')

image
I also added gridlines, but I noticed other examples don't have them, so just my personal preference, dk what the convention is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback Mike! Made these changes and am uploading a new version now :)

Copy link
Member

@wholmgren wholmgren left a comment

Choose a reason for hiding this comment

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

Thanks @ericf900 for the great example and @kanderso-nrel @mikofski @cwhanse for the reviews. I suggest merging as is.

@wholmgren wholmgren changed the title Create ghi_transposition.py Create plot_ghi_transposition.py example Mar 17, 2020
@wholmgren wholmgren merged commit e526b55 into pvlib:master Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

5 participants