calculate surface angles using unit normal#2702
Conversation
| surface_azimuth = np.degrees( | ||
| np.arctan2(unit_normal[:, 0], unit_normal[:, 1])) |
There was a problem hiding this comment.
I would include this line in the function that calculates the unit normal so that it is self-contained w.r.t. its conventions.
There was a problem hiding this comment.
Are you suggesting that _unit_normal calculate and return surface_azimuth? I'm unclear what you are suggesting.
There was a problem hiding this comment.
That was the thought, yes. Angles in, angle out is independent of the method. I guess the function name would change too.
There was a problem hiding this comment.
Thanks for the suggestion. I guess I'm preferring _unit_normal as the function because it may find other uses. We could package the few lines here into a helper _calc_surface_azimuth but I don't see that as much of an improvement.
There was a problem hiding this comment.
The function name seems very general and I initially suspected it could indeed have other intended uses; but the calculations seem very specialized, which raised some doubt. What other uses do you have in mind?
There was a problem hiding this comment.
I am assuming you are referring to the _unit_normal function. In this instance it is specific to a surface mounted on a single-axis tracker. I am thinking that having the unit normal for that surface enables other computing angles in a 3D scene, which are much easier to write as dot products than using trig functions. pvlib doesn't do much of that, currently. This function could be used in place of the current AOI calculation, but I don't see any advantage to do so.
There was a problem hiding this comment.
Yes indeed, having the unit normal for the single axis tracker surface could be used for other subsequent calculations, such as calculating surface_tilt or AOI. But the _unit_normal function itself does three tracker-specific rotations in a predetermined sequence, so I don't see how that can be repurposed.
If you did use unit_normal to calculate both surface_tilt and surface_azimuth, then that would make a better case for current the division of labor between function and helper, and maybe also serve as a better example for future uses of the vector approach.
echedey-ls
left a comment
echedey-ls
left a comment
There was a problem hiding this comment.
I've compared _unit_normal with an implementation I did based on scipy rotation objects, and they match. The 3d axis basis is the same E-N-Up, left-hand rotations.
I just left this comment in the issue, I hope not to delay a fix on negative tilt handling.
pvlib/tracking.py
Outdated
| Unit normal to rotated tracker surface, in global E-N-Up coordinates, | ||
| given by R*(0, 0, 1)^T, where: | ||
|
|
||
| R = Rz(-axis_azimuth) Rx(-axis_tilt) Ry(theta) * |
There was a problem hiding this comment.
| R = Rz(-axis_azimuth) Rx(-axis_tilt) Ry(theta) * | |
| R = Rz(-axis_azimuth) Rx(-axis_tilt) Ry(theta) |
pvlib/tracking.py
Outdated
| rotation by -axis_tilt about the x-axis, where axis_tilt is negated | ||
| because pvlib's convention is that the positive y-axis is tilted | ||
| downwards. Ry is a rotation by theta | ||
| about the y-axis. theta is negated so that a negative. |
There was a problem hiding this comment.
I think this sentence was not ended correctly, right?
| example, for a tracker with ``axis_azimuth``=180 and ``axis_tilt``=10, | ||
| the north end is higher than the south end of the axis. | ||
|
|
||
| axis_azimuth : float, default 0 |
There was a problem hiding this comment.
All the examples refer to azimuth 180, so why is the default zero?
There was a problem hiding this comment.
I don't think there's a particular reason for the default of 0. It's the least common orientation in the northern hemisphere but perhaps its most common in the southern hemisphere. 0 has been the default for a long time, so maybe inertia wins? Examples are different question, though.
| @@ -84,7 +84,7 @@ def singleaxis(apparent_zenith, solar_azimuth, | |||
| intersection between the slope containing the tracker axes and a plane | |||
There was a problem hiding this comment.
I haven't looked at these functions much before, and it took me a while to realize that "slope" usually refers to the terrain or ground itself, or in this case surface floating above the ground, rather than an attribute of a line or surface. I the documentation could be improved by changing "slope" to "sloped surface" or "sloped ground"or "sloped terrain" or similar depending on the context.
If this gets a few thumbs up I can make specific suggestions.
There was a problem hiding this comment.
I agree that this description can be improved. "slope" is not specific. The precise definition should describe an intersection of two planes (because that's how a line is defined, and lines define angles). I think "slope" is intended to mean the plane that contains the tracker axes. @kandersolar
There was a problem hiding this comment.
I think "slope" is intended to mean the plane that contains the tracker axes.
Right. +1 to improved wording, although maybe that clarification should be separate from this PR.
4 participants
docs/sphinx/source/referencefor API changes.docs/sphinx/source/whatsnewfor 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`).remote-data) and Milestone are assigned to the Pull Request and linked Issue.Changes the calculation of surface angles for tracking.singleaxis to allow for negative axis_tilt.
Changes the output of singleaxis in the case of surface_tilt=0, to consistently return surface_azimuth=axis_azimuth.