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

Clarify docstring descriptions of cross-axis slope #1530

Merged
merged 5 commits into from
Sep 14, 2022

Conversation

chiragpachori
Copy link
Contributor

@chiragpachori chiragpachori commented Aug 16, 2022

The previous description for the signs of cross axis tilt mentioned slope azimuth as east for both positive and negative. Corrected it to indicate that sign of cross axis tilt is positive when axis azimuth is south and slope is towards west.

This is a minor documentation error and I used the tracking.calc_cross_axis_tilt function to make sure that I an getting positive sign when slope is facing west.

The previous description for the signs of cross axis tilt mentioned slope azimuth as east for both positive and negative. Corrected it to indicate that sign of cross axis tilt is positive when axis azimuth is south and slope is towards west.
@cwhanse
Copy link
Member

cwhanse commented Aug 16, 2022

We're all for clarifying docstrings.

The existing docstring says "negative cross-axis tilt if the tracker axes plane slopes down to the east and positive
cross-axis tilt if the tracker axes plane slopes up to the east" (emphasis mine) which isn't contradictory but perhaps is difficult to parse.

Would "positive cross-axis tilt if the tracker axes plane slopes down to the west" be more clear?

@chiragpachori
Copy link
Contributor Author

chiragpachori commented Aug 17, 2022

Ohh I see! Apologies for missing that part.

I think phrasing it as "positive cross-axis tilt if the tracker axes plane slopes down to the west" might make it more clear. As it will line up well with the other definitions throughout the documentation, where sloping down is generally used to to get the direction. For example (from the same file):

  1. slope_azimuth : float
    direction of the normal to the slope containing the tracker axes, when
    projected on the horizontal [degrees]
  2. surface_azimuth: The azimuth of the rotated panel, determined by
    projecting the vector normal to the panel's surface to the earth's
    surface. [degrees]

In both these cases the intuitive reference is sloping down, at least from how I understand it.

@cwhanse
Copy link
Member

cwhanse commented Aug 17, 2022

the intuitive reference is sloping down

I agree.

@cwhanse
Copy link
Member

cwhanse commented Aug 26, 2022

@chiragpachori will you be able to update this PR? If not, one of us can do it, just let us know.

@kandersolar kandersolar added this to the 0.9.3 milestone Sep 1, 2022
@kandersolar kandersolar modified the milestones: 0.9.3, 0.9.4 Sep 14, 2022
@cwhanse cwhanse mentioned this pull request Sep 14, 2022
@kandersolar kandersolar modified the milestones: 0.9.4, 0.9.3 Sep 14, 2022
cwhanse and others added 2 commits September 14, 2022 09:02
Co-authored-by: Kevin Anderson <kevin.anderson@nrel.gov>
Co-authored-by: Kevin Anderson <kevin.anderson@nrel.gov>
@kandersolar kandersolar changed the title Description for tracking.calc_cross_axis_tilt Clarify docstring descriptions of cross-axis slope Sep 14, 2022
@kandersolar kandersolar merged commit ce7ddbc into pvlib:master Sep 14, 2022
@kandersolar
Copy link
Member

Thanks @chiragpachori!

@chiragpachori
Copy link
Contributor Author

@cwhanse Sorry I missed your comment. Thank you for sorting it out. @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.

3 participants