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

Closed
MichalArieli opened this issue Jun 18, 2023 · 4 comments · Fixed by #1852
Closed

Add min_angle argument to tracking.singleaxis #1777

MichalArieli opened this issue Jun 18, 2023 · 4 comments · Fixed by #1852
Milestone

Comments

@MichalArieli
Copy link
Contributor

In tracking.singleaxis the minimum angle of the tracker is assumed to be opposite of the maximum angle, although in some cases the minimum angle could be different. NREL SAM doesn't support that but PVsyst does.

In order to support non symmetrical limiting angles, tracking.singleaxis should have another, optional, input, min_angle. By default, if not supplied (i.e. value is None), the current behavior (min_angle = -max_angle) would apply.

Can I propose a PR for this, with modifications to tracking.singleaxis, tracking.SingleAxisTracker and to pvsystem.SingleAxisTrackerMount + corresponding tests?

@kandersolar
Copy link
Member

I'm in favor of pvlib being able to handle asymmetrical rotation limits in principle, but I'm curious what situation has that asymmetry in practice. @MichalArieli do you have a particular real-world application in mind?

Rather than separate min_ and max_ parameters, I think I'd favor a single parameter that accepts a tuple as @cwhanse suggested here: #823 (comment). I'm not sure about renaming max_angle to something else though. singleaxis(..., max_angle=(-40, 45)) seems okay to me. And since symmetrical limits is by far the more common case, I think the parameter should continue accepting a single value (in which case symmetry is assumed) in addition to a tuple.

tracking.SingleAxisTracker is being removed anyway (#1771), so no point in making any additions there. Whatever changes we decide on here should only be made to tracking.singleaxis and pvsystem.SingleAxisTrackerMount (and tests, of course).

@MichalArieli
Copy link
Contributor Author

@kandersolar Thanks for the quick response!

Regarding handling asymmetry in rotation limits, let's take the example of a tracker placed at a 90-degree axis azimuth, tracking south-north. If the sun is at azimuth 80 degrees during sunrise, the algorithm will guide the tracker to briefly turn north. To prevent that we can implement a maximum angle limit for northward movement to ensure smooth and continuous motion and taking into account the time needed for such a large angular change.

I agree its better to have a single parameter that accepts a tuple/ single value. Would you like me to apply these changes and send for a PR?

@kandersolar
Copy link
Member

Would you like me to apply these changes and send for a PR?

Please do!

@jules-ch
Copy link
Contributor

I feels like naming it min_angle is ambiguous, idk if it is juste me.
Feels like a max angle range.
Anyway the keyword won't be changed, just my 2 cents on the terminology.

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