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

Allow asymmetrical rotation limits in pvlib.tracking.singleaxis #1809

Closed

Conversation

MichalArieli
Copy link
Contributor

@MichalArieli MichalArieli commented Jul 18, 2023

  • Closes Add min_angle argument to tracking.singleaxis #1777
  • 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.

Modify max_angle to accept tuples in addition to single values
(issue #1777)

In tracking.singleaxis the minimum angle of the tracker is always the opposite of the maximum angle. However, there are cases where the minimum angle can differ. (While NREL SAM does not support this feature, PVsyst does).

To address this limitation and support non-symmetrical limiting angles, I have made a modification to the min_angle parameter in tracking.singleaxis. It now accepts tuples of the form (min_angle, max_angle). To maintain compatibility with the more common symmetrical limits, the parameter still allows a single value.

Please review the changes. Thank you!

@MichalArieli MichalArieli changed the title Modify max_angle to include min_angle Modify max_angle to include min_angle #1777 Jul 18, 2023
@MichalArieli MichalArieli changed the title Modify max_angle to include min_angle #1777 Modify max_angle to include min_angle Jul 18, 2023
@cwhanse
Copy link
Member

cwhanse commented Jul 18, 2023

@MichalArieli I have no idea why GH is saying there are conflicts. Can you merge the current main from pvlib and push again?

@MichalArieli
Copy link
Contributor Author

MichalArieli commented Jul 18, 2023

@MichalArieli I have no idea why GH is saying there are conflicts. Can you merge the current main from pvlib and push again?

@cwhanse I did it. I think the conflict is still there. I'll try to solve it tomorrow (probably I'll just fork again and apply my changes). Thank you!

@kandersolar kandersolar added this to the v0.10.2 milestone Jul 19, 2023
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.

@MichalArieli please add a comment to docs/sphinx/source/whatsnew/v0.10.2.rst, and yourself to the list of contributors.

Thanks again for the contribution.

rotation angle, and the minimum rotation angle is assumed to be the
opposite of the maximum angle. If a tuple of (min_angle, max_angle)
is provided, it represents both the minimum and maximum rotation angles.

Copy link
Member

@cwhanse cwhanse Jul 19, 2023

Choose a reason for hiding this comment

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

I feel like we need to explain the orientation that distinguishes min_angle from max_angle. Suggest adding a new paragraph like:

A rotation to 'max_angle' is a counter-clockwise rotation about the y-axis of the tracker coordinate system. For example, for a tracker with 'axis_azimuth' oriented to the south, a rotation to 'max_angle' is towards the west, and a rotation toward 'min_angle' is in the opposite direction, toward the east.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for this input! As you suggested, I have added this paragraph.

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 @MichalArieli

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 @MichalArieli, this looks great! A couple nitpicks below.

One other thing: our automated linter is currently broken (#1722) so there's unfortunately no CI check for this currently, but could you re-wrap lines in .py files to 80 chars max?


# Determine minimum and maximum rotation angles for the tracker based on max_angle.
# If max_angle is a single value, assume min_angle is the negative of max_angle.
if np.array(max_angle).size == 1:
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
if np.array(max_angle).size == 1:
if np.isscalar(max_angle):

Maybe a bit easier to read while achieving the same thing?

Comment on lines +18 to +20
* Added support for asymmetric limiting angles in :py:func:`pvlib.tracking.singleaxis`, by modifying
the `min_angle` parameter to accept tuples.
(:pull:`1809`)
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
* Added support for asymmetric limiting angles in :py:func:`pvlib.tracking.singleaxis`, by modifying
the `min_angle` parameter to accept tuples.
(:pull:`1809`)
* Added support for asymmetric limiting angles in :py:func:`pvlib.tracking.singleaxis` and
:py:class:`pvlib.pvsystem.SingleAxisTrackerMount` by modifying the ``max_angle``
parameter to accept tuples. (:issue:`1777`, :pull:`1809`)

@adriesse
Copy link
Member

The title of this PR is quite interesting...

@kandersolar kandersolar changed the title Modify max_angle to include min_angle Allow asymmetrical rotation limits in pvlib.tracking.singleaxis Sep 8, 2023
@kandersolar
Copy link
Member

Superseded by #1852. Thanks again @MichalArieli!

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.

Add min_angle argument to tracking.singleaxis
4 participants