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 cross-axis slope backtracking #1077

Merged
merged 13 commits into from
Oct 9, 2020

Conversation

kandersolar
Copy link
Member

@kandersolar kandersolar commented Oct 7, 2020

  • Closes #xxxx
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries to docs/sphinx/source/api.rst 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 and Milestone are assigned to the Pull Request and linked Issue.
  • Possible to-do: remove tracking.ipynb per Singleaxis tracking with non-parallel slope #823 (comment)

As suggested in #823, here's a gallery example attempting to clarify the various angles and sign conventions in use. The static images are custom (using asymptote) for this example and can easily be modified; I am limited more by tunnel vision and lack of imagination than by the tool and welcome feedback to improve them.

rendered example: https://pvlib-python--1077.org.readthedocs.build/en/1077/auto_examples/plot_backtracking_sloped_terrain.html

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.

This is quite clear, except at one point, but I have the advantage of having worked through the reference and the code already.

docs/examples/plot_backtracking_sloped_terrain.py Outdated Show resolved Hide resolved
docs/examples/plot_backtracking_sloped_terrain.py Outdated Show resolved Hide resolved
docs/examples/plot_backtracking_sloped_terrain.py Outdated Show resolved Hide resolved
docs/examples/plot_backtracking_sloped_terrain.py Outdated Show resolved Hide resolved
docs/examples/plot_backtracking_sloped_terrain.py Outdated Show resolved Hide resolved
Comment on lines 130 to 131
slope_azimuth = 155 # terrain slopes down to the south-south-east
slope_tilt = 10 # terrain is sloped at 10 degrees from horizontal
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused at this point. The slope direction is 155 azimuth, but, the tilt is 10 degrees relative to what? Is it 10 degrees rotation relative to the tracker azimuth of 180? That seems an odd choice, since it is difficult to measure the slope tilt except relative to the slope azimuth.

If slope_tilt is 10 degrees relative to horizontal along slope_azimuth, then I'd suggest

Suggested change
slope_azimuth = 155 # terrain slopes down to the south-south-east
slope_tilt = 10 # terrain is sloped at 10 degrees from horizontal
slope_tilt = 10 # terrain slopes upward 10 degrees from horizontal in the direction of slope_azimuth

Copy link
Member Author

Choose a reason for hiding this comment

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

If slope_tilt is 10 degrees relative to horizontal along slope_azimuth,

Correct, but I think your suggested comment has the wrong sign. It's either upwards 10 degrees away from slope_azimuth, or downward 10 degrees towards it. Are you okay with this revision?

terrain slopes upward 10 degrees from horizontal away from the direction of slope_azimuth

Copy link
Member

Choose a reason for hiding this comment

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

slope_azimuth is chosen in the downslope direction? I may have missed that if it's earlier in the text. If not, would be good to add. If slope_azimuth is always downslope, then I think it may be more clear to describe slope_tilt in the direction of slope_azimuth, i.e., always negative.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for your patience with this reviewer

Copy link
Member Author

Choose a reason for hiding this comment

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

Changes made. As always, I am grateful for the review :)

docs/examples/plot_backtracking_sloped_terrain.py Outdated Show resolved Hide resolved
docs/examples/plot_backtracking_sloped_terrain.py Outdated Show resolved Hide resolved
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.

LGTM and thank you @kanderso-nrel

@wholmgren wholmgren added this to the 0.8.1 milestone Oct 8, 2020
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 @kanderso-nrel, this is very helpful. I commented on a few minor things as I read through it, but I'm ok with merging this anytime.

Would it make sense to link to the other single axis tracker example or at least put them next to each other in the menu? I don't know how that's configured.

Maybe wait to delete tracking.ipynb until we have a couple of examples for parameters like gcr and max_angle (probably in the simple single-axis tracker example).

Backtracking on sloped terrain
==============================

Modeling backtracking for single-axis tracker arrays on sloped terrain.
Copy link
Member

Choose a reason for hiding this comment

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

This reads like a module docstring but renders as regular text, so I think the rendered introduction is awkward.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed it's awkward in the rendered page. The reason for the strange wording is the docstring-style line is also used as the hover text for the thumbnail in the gallery listing, which has to be brief. Open to suggestions for text that works better in both contexts!

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like a gallery configuration problem. Let's ignore.

# sun is low in the sky. The backtracking strategy orients the modules exactly
# on the boundary between shaded and unshaded so that the modules are oriented
# as much towards the sun as possible while still remaining unshaded.
# Unlike the truetracking calculation (which only depends on solar position),
Copy link
Member

Choose a reason for hiding this comment

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

Is "truetracking" (one word) widely accepted? I'd probably write true-tracking even at the cost of inconsistency with backtracking.

Copy link
Member

Choose a reason for hiding this comment

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

The existing single-axis tracker example uses the hyphenated version.

gcr=gcr,
cross_axis_tilt=cross_axis_tilt)

backtracking_position = tracker_data['tracker_theta'].fillna(0)
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
backtracking_position = tracker_data['tracker_theta'].fillna(0)
# tracker rotation is undefined at night
backtracking_position = tracker_data['tracker_theta'].fillna(0)

# calculate the solar position
times = pd.date_range('2019-01-01 06:00', '2019-01-01 18:00', closed='left',
freq='1min', tz=tz)
solpos = solarposition.get_solarposition(times, lat, lon)
Copy link
Member

Choose a reason for hiding this comment

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

The rendered version has some weird syntax highlighting inconsistencies. I don't see anything wrong so probably safe to ignore.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you're talking about things being rendered blue-ish with clickable links, I think that's the intersphinx linking: https://sphinx-gallery.github.io/stable/configuration.html#add-intersphinx-links-to-your-examples

Future improvement: configure pvlib functions to link as well, or disable it for everything, for consistency.

Copy link
Member

Choose a reason for hiding this comment

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

ohhh, cool, I didn't even realize they were links. I've recently visited some of the links but not all, so it's a mix of purple and blue. Ok to ignore.

Screen Shot 2020-10-08 at 11 25 42 AM

@kandersolar
Copy link
Member Author

Would it make sense to link to the other single axis tracker example or at least put them next to each other in the menu? I don't know how that's configured.

I think related examples can be grouped into sections with folders, and besides that there is ordering functionality: https://sphinx-gallery.github.io/stable/configuration.html#sorting-gallery-subsections

Preference on introducing gallery sections here (e.g. "Weather", "PV Modeling", "Tracking") vs leaving that for another day and linking to the other example for now?

Maybe wait to delete tracking.ipynb until we have a couple of examples for parameters like gcr and max_angle (probably in the simple single-axis tracker example).

Fine by me.

@cwhanse
Copy link
Member

cwhanse commented Oct 8, 2020

Preference on introducing gallery sections here (e.g. "Weather", "PV Modeling", "Tracking") vs leaving that for another day and linking to the other example for now?

Was just thinking the same thing, that categories would help organize the examples.

@wholmgren
Copy link
Member

I think related examples can be grouped into sections with folders, and besides that there is ordering functionality: https://sphinx-gallery.github.io/stable/configuration.html#sorting-gallery-subsections

Preference on introducing gallery sections here (e.g. "Weather", "PV Modeling", "Tracking") vs leaving that for another day and linking to the other example for now?

Thanks. Up to you. Changing this file name to plot_single_axis_tracking_on_sloped_terrain.py would also mitigate my immediate concern.

@mikofski
Copy link
Member

mikofski commented Oct 9, 2020

This looks great! Thanks Kevin!

@wholmgren
Copy link
Member

test failure is unrelated. thanks @kanderso-nrel!

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