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

Singleaxis tracking with non-parallel slope #823

Merged
merged 46 commits into from Sep 5, 2020

Conversation

mikofski
Copy link
Member

@mikofski mikofski commented Nov 22, 2019

N-S singleaxis trackers on an E-W slope

  • as discussed in google groups trackers on slopes
  • I am familiar with the contributing guidelines.
  • Fully tested. Added and/or modified tests to ensure correct behavior for all reasonable inputs. Tests (usually) must pass on the TravisCI and Appveyor testing services.
  • Updates entries to docs/sphinx/source/api.rst for API changes.
  • Adds description and name entries in the appropriate docs/sphinx/source/whatsnew file for all changes.
  • Code quality and style is sufficient. Passes LGTM and SticklerCI checks.
  • New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
  • Pull request is nearly complete and ready for detailed review.

Brief description of the problem and proposed solution (more info in google groups post linked to above):

  • adds 2 new functions:
    • pvlib.tracking.calc_tracker_axis_tilt(system_azimuth, system_zenith, axis_azimuth) -> axis_tilt
    • pvlib.tracking.calc_system_tracker_side_slope(axis_azimuth, axis_tilt, system_azimuth, system_zenith) -> side_slope, relative_rotation
  • adds 1 new parameter, side_slope to pvlib.tracking.singleaxis with default 0.

Example 1: to model a N-S tracker on an east west slope, simple set axis tilt to zero, axis azimuth to 180, and system plane to (90, side slope), and set the side slope to what ever you want the E-W slope to be. Voila!

Example 2: you can model more complicated slopes in any direction.

  1. define a system plane as (sys_az, sys_ze). this is the plane that contains the tracker axes.
  2. Use calc_tracker_axis_tilt to back out the tracker axis tilt given its azimuth
  3. Use calc_system_tracker_side_slope to back out the slope perpendicular to the tracker axes
  4. use pvlib.tracking.singleaxis with the calculated tracker axis tilt and side slope from steps 2 & 3
    Voila!

- add calc_axis_tilt and calc_side_slope functions to tracking.py
- add tests for trackers on slopes
- add singleaxis tracker w/slope test data

Signed-off-by: Mark Mikofski <bwana.marko@yahoo.com>
- match pvlib algorithms - despite disagreement
- remove unused _get_solar_vector, oops! not part of this pr, was
 decreasing coverage
@mikofski
Copy link
Member Author

note: the travis py35-min is failing because the numpy requirement is 1.10.4, when np.roll() was restricted to integers and one axis at a time, but since 1.12.0 it can roll over multiple axes, see Notes section

@mikofski
Copy link
Member Author

PTAL @jforbess and @kevinsa5 thx!

pvlib/tracking.py Outdated Show resolved Hide resolved
@kevinsa5
Copy link
Contributor

kevinsa5 commented Dec 3, 2019

I think your backtracking correction is a little different from the one I derived. It's not immediately clear to me, but adding side_slope to wid in temp = np.clip(axes_distance*cosd(wid + side_slope), -1, 1) looks like a rotation of one row around another, which would change the horizontal axes_distance a bit. I'll take another look at my version and see if I can spot an error in the math.

Either way, I'm not sure I understand the max_angle shifting. Is the idea that the tracker motors are installed facing away from the slope so that their zero-point is now off-vertical? Is that something that installers will actually do?

@mikofski
Copy link
Member Author

mikofski commented Dec 3, 2019

I think your backtracking correction is a little different from the one I derived.

I am using the existing backtracking algorithm in pvlib.tracking.singleaxis. This new PR just transforms everything into the reference frame of a "system plane" which is defined as the surface that contains all trackers axes. Once the solar vector and trackers axes are described in the system plane reference frame, then the existing pvlib.tracking.singleaxis true-tracking and back-tracking algorithms work just as normal.

It's not immediately clear to me, but adding side_slope to wid in temp = np.clip(axes_distance*cosd(wid + side_slope), -1, 1) looks like a rotation of one row around another, which would change the horizontal axes_distance a bit.

I didn't intend it to be "a rotation of one row around another". My intention of adding the side slope to the rotation is transform the reference frame from horizontal (or tilted tracker axis ref. frame) to the system plane. Because the side slope is perpendicular to the tracker axes, then adding it to the tracker rotation just makes it relative to the system plane reference frame. EG: for NS trackers on an eastern slope of 9[deg], an 11[deg] tracker rotation in the horizontal plane would be 20[deg] (or 2[deg] depending on the tracker azimuth) in the reference frame of the eastern slope.

I'm not familiar with the axes_distance parameter, is it part of pvlib.tracking.singleaxis? This algorithm uses the GCR which is defined as the ratio of the length of the PV surface to the distance between the trackers. The way I interpret this is if NS trackers 2[m] long were 10[m] apart on an eastern slope of 9[deg], then the GCR is 20%. I can't tell if you're implying that the distance between the trackers is only measured in the horizontal, but if so, that's not my interpretation. AFAIK this distance is measured in the system plane directly from one tracker axis to the next tracker.

I'll take another look at my version and see if I can spot an error in the math.

Please do point out any errors! Thanks!

Either way, I'm not sure I understand the max_angle shifting.

The tracker_theta is shifted to the system plane when applying the max_angle because pvlib assumes symmetrical max and min rotation angles, and so I believe the best way to handle the max_angle parameter is to treat it in the system plane exactly the same way it would be handled if the system plane were horizontal, IE: symmetrical about the normal to the system plane. I actually think this is consistent with the way that max_angle is currently handled for tilted tracker axes. The max_angle for a tilted tracker is not interpreted relative to the horizontal, but relative to the tracker axis. Ditto here, just in a different direction.

Is the idea that the tracker motors are installed facing away from the slope so that their zero-point is now off-vertical? Is that something that installers will actually do?

It would appear that way, but this is only for convenience. This algorithm doesn't require trackers to be installed in any particular way at all. However, unless pvlib changes the max_angle to be two separate inputs, [min_angle, max_angle] this is the best way I can think of to correctly handle a symmetrical max_angle input. Note that tracker_theta is adjusted back to the horizontal so the zero point is still vertically upward. It's just the max_angle that is interpreted in the reference frame of the system plane. EG a max_angle of 30[deg] on a 9[deg] slope would be a minimum and maximum rotation of (-21[deg], 39[deg]) in the horizontal reference frame.

@kandersolar
Copy link
Member

kandersolar commented Dec 3, 2019

Thanks for the explanations! The axes_distance value is just an implementation detail here, equal to 1/GCR.

I can't tell if you're implying that the distance between the trackers is only measured in the horizontal, but if so, that's not my interpretation. AFAIK this distance is measured in the system plane directly from one tracker axis to the next tracker.

I was indeed assuming that axis spacing (and GCR) are measured horizontally. I've been thinking about tracking arrays on side slopes as just arrays on flat terrain where each row has been raised vertically compared to its neighbor -- i.e. horizontal distance between rows is what defines the array geometry and is independent of slope. This just comes from my limited experience in utility scale development where the array was designed and modeled assuming flat terrain, so the system GCR was calculated using horizontal distance and the array built accordingly. Is there a convention for GCR to be calculated using slant distance, maybe from the rooftop world?

this is the best way I can think of to correctly handle a symmetrical max_angle input

I see the mathematical appeal of tracker limits being relative to the system plane, but I think the more common use case for nonzero side_slope is to still have max_angle be relative to vertical (or tilted vertical for tilted axes). I don't really have any hands-on experience on this but I'd guess that most tracker hardware expects to be mounted to the mounting post such that their zero point is in the vertical plane through the tracker axis, and as such their maximum rotation angles would be relative to vertical. This is assuming that people use the pvlib max_angle input as the physical motor rotation limit. Although maybe it wouldn't be a bad thing to add the option of [min_angle, max_angle] anyway since it would be useful in some cases outside of sloped arrays as well.

Edit: gotta love having two github accounts and forgetting to switch between them...

@mikofski
Copy link
Member Author

mikofski commented Dec 3, 2019

Thanks Kevin-NREL 😉

I was indeed assuming that axis spacing (and GCR) are measured horizontally

I think we can accommodate this, since we know the side slope then with pitch_horz being defined as measured horizontally, and p_prime as measured along the side slope

axes_distance = 1/GCR = pitch_horz / length  = p_prime * cos(side_slope) / length

therefore in the backtracking condition becomes (existing algorithm):

temp = np.clip(axes_distance/cos(side_slope) * cosd(wid + side_slope), -1, 1)

or with #824, uses abs(cos(wid)):

temp = np.minimum(axes_distance/cos(side_slope) * np.abs(cosd(wid + side_slope)), 1.0)

Since you've worked for a developer, I defer to your judgment, but it would be very useful to get others to chime in here (@jforbess , @cwhanse , any others?). There may be flexibility to choose what we want if there is no existing convention, since trackers on terrain is kind of a newish concept. EG at SunPower I believe we assumed the sites were flat (or within 3-deg of flat which is nearly the same thing, cos(3) = cos(-3) = 0.999)

@mikofski
Copy link
Member Author

mikofski commented Dec 3, 2019

the more common use case for nonzero side_slope is to still have max_angle be relative to vertical (or tilted vertical for tilted axes)

I think we can do this too, just remove the 1st and last lines here. Perhaps we can consider if this is the best convention if there isn't yet a popular consensus.

This is assuming that people use the pvlib max_angle input as the physical motor rotation limit.

In addition to the drive motor limits, I thought tracker rotation might be limited by the ground, if it's not high enough to rotate to 90°

@kevinsa5
Copy link
Contributor

kevinsa5 commented Dec 3, 2019

Agreed that input from others on conventions would be useful.

axes_distance/cos(side_slope ) * cosd(wid + side_slope) is just a trig identity away from what I have: (cos(wid) - grade*sin(wid))/gcr, where grade = tan(side_slope) = vertical_offset / horizontal_offset. Very cool! I love how these equations simplify down to need only unitless parameters.

I thought tracker rotation might be limited by the ground, if it's not high enough to rotate to 90°

Good point. How about adding a new parameter min_angle=None and a guard at the top of the function:

if min_angle is None:
    min_angle = -max_angle

@cwhanse
Copy link
Member

cwhanse commented Dec 3, 2019

Agreed that input from others on conventions would be useful.

I thought tracker rotation might be limited by the ground, if it's not high enough to rotate to 90°

Good point. How about adding a new parameter min_angle=None and a guard at the top of the function:
if min_angle is None:
min_angle = -max_angle

I'm not in favor of min_angle and max_angle as names. I'd vote to change to rotation_limit (either as a tuple, or a pair _min and _max, I suppose rotation_limit = 45 could communicate +/-45 but I don't think this convenience is worth handling the different types - easy enough to supply rotation_limit = (-45, 45).

In either case, the rotation angle sign is referenced to the coordinate system described in the docstring. I had a conversation with a few people in the industry about conventions for rotation angle and azimuth - I heard a consensus that rotation of 0 degrees meant the module is horizontal, and negative rotation angle is generally thought of as the "morning" direction (east for N-S oriented trackers). As for whether a N-S tracker has azimuth 0 or 180, not a consensus.

Currently, the rotation angle is viewed by looking in the direction of the tracker azimuth (positive y-axis in the coordinate system). In hindsight, I should have set a positive rotation as counter-clockwise about the y-axis, rather than positive being clockwise, to describe rotation as an angle. I was working too hard to maintain a connection to the reference (which assumes tracker azimuth points towards the equator) and also have negative rotation angles towards the east.

@mikofski
Copy link
Member Author

mikofski commented Dec 4, 2019

Thanks Cliff!

In either case, the rotation angle sign is referenced to the coordinate system described in the docstring. I had a conversation with a few people in the industry about conventions for rotation angle and azimuth - I heard a consensus that rotation of 0 degrees meant the module is horizontal

I want to make sure this is completely clear, this PR does not change the rotation convention, and it uses the same algorithm as already exists in pvlib.tracking.singleaxis. A tracker_theta of zero still means horizontal, face up. This does not change.

Regarding the max_angle I'm happy to go with the consensus that it is defined from the zero-point rotation.

Regarding adding a min_angle or letting users provide either a tuple or scalar - I propose we leave that for another PR - ok?

@mikofski mikofski mentioned this pull request Dec 4, 2019
@cwhanse
Copy link
Member

cwhanse commented Dec 4, 2019

I want to make sure this is completely clear, this PR does not change the rotation convention, and it uses the same algorithm as already exists in pvlib.tracking.singleaxis. A tracker_theta of zero still means horizontal, face up. This does not change.

I'm open to changing it so that rotations are angles in the usual sense, not in this PR's scope.

Regarding adding a min_angle or letting users provide either a tuple or scalar - I propose we leave that for another PR - ok?

OK with me.

@mikofski
Copy link
Member Author

mikofski commented Dec 4, 2019

OK, one more question. Is the GCR - is it based strictly on the horizontal distance between rows or not?

EG:

GCR = surf_len / horz_dist = surf_len / dist_prime / cos(side_slope)

Where:

  • surf_len is the length of the PV surface perpendicular to the tracker axes,
  • horz_dist = dist_prime*cos(side_slope) is the projection of dist_prime on the horizontal and
  • side_slope is the angle of the "system plane" perpendicular to the tracker axes:

@wholmgren wholmgren mentioned this pull request Dec 6, 2019
8 tasks
@kevinsa5
Copy link
Contributor

I asked around some people I know in development engineering/performance engineering and 3/3 said that GCR is based on horizontal distance. N=3 isn't a great sample size, but some data is better than no data...

Two thoughts:

  • People are used to thinking of system layout in the top-down CAD view and construction crews are going to drive piles according to the horizontal spacing in those drawings.
  • GCR is often a way of quantifying "how much DC am I fitting onto this parcel?" and since parcel acreage is measured on the horizontal plane, a horizontal GCR is consistent with that usage.

@mikofski
Copy link
Member Author

Thanks! That makes a lot of sense. I'll ask around here, and see if people agree. Then I think it's okay to change gcr to horizontal. Then, will our two models will be the same?

@kandersolar
Copy link
Member

I still get one invalid warning here (from the older code): https://github.com/mikofski/pvlib-python/blob/6ed62ff68f9d6234c994274e8a92b22fc17e9bb5/pvlib/tracking.py#L607

Whatsnew entry for the new side_slope parameter in SingleAxisTracker, I guess?

Otherwise good to merge from my perspective! 🚀

@wholmgren
Copy link
Member

What's the rationale for #1041 as future work instead of deleting all the outdated and unneeded comments now? No comments is better than wrong comments.

@mikofski
Copy link
Member Author

mikofski commented Sep 3, 2020

@wholmgren how much pruning is okay? Should I be greedy, conservative? Trust my instinct?

@wholmgren
Copy link
Member

I trust your instincts!

* with new side_slope arg in SingleAxisTracker class
* add fix for low sun angle tracker rotation calculations (issue 824)
* add links to pr 823
* ignore invalid warnings for nan > x and nan % x in tracking.py
* remove commented code, especially any code relating to use of arctan2
  adopted in 1fb82cc
* remove comments that should be sourced directly from refernces, and
  instead list reference and Eqs. numbers
* tidy up, combine lines, etc
@mikofski
Copy link
Member Author

mikofski commented Sep 3, 2020

  • @kanderso-nrel 👀, I found 2 invalid warnings, the one you pointed out (NaN % real) and one I had removed by incorrectly (NaN > real) -- I was testing with the validation dataset from the paper which has no NaN's, so that's why I didn't see them. Thanks!
  • I also added the 🐞 fix for low sun angles, issue 824 to what's new
  • @wholmgren 👀, I was a bit overzealous in pruning the comments. Hard balance I find, but I erred on the side of just updating the equation numbers to point to the newer NREL tech report, which IMHO is much easier to follow 😉

OK maybe now? 🚀

@mikofski
Copy link
Member Author

mikofski commented Sep 3, 2020

I know I should shut my mouth, but is "tracker" redundant in calc_tracker_axis_tilt and calc_system_tracker_side_slope? Shouldn't it just be: calc_axis_tilt & calc_system_side_slope? I mean b/c they're both already in the pvlib.tracking namespace.

Hopefully quick fix using rename symbols, better now than later, right?

thanks!

@wholmgren
Copy link
Member

is "tracker" redundant in calc_tracker_axis_tilt and calc_system_tracker_side_slope? Shouldn't it just be: calc_axis_tilt & calc_system_side_slope?

I agree

* in api.rst
* what's new v0.8.0
* tracking.py & test_tracking.py
@mikofski
Copy link
Member Author

mikofski commented Sep 3, 2020

LGTM

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.

I agree with @kanderso-nrel let's rip that hairy bandaid off and consider this a wholesale replacement of pvlib.tracking.

pvlib/tracking.py Outdated Show resolved Hide resolved
pvlib/tracking.py Outdated Show resolved Hide resolved
pvlib/tracking.py Show resolved Hide resolved
pvlib/tracking.py Outdated Show resolved Hide resolved
pvlib/tracking.py Outdated Show resolved Hide resolved
pvlib/tracking.py Outdated Show resolved Hide resolved
pvlib/tracking.py Outdated Show resolved Hide resolved
pvlib/tracking.py Show resolved Hide resolved
pvlib/tracking.py Show resolved Hide resolved
pvlib/tracking.py Outdated Show resolved Hide resolved
mikofski and others added 2 commits September 3, 2020 11:45
* side slope is line in system plane, perp. to tracker azimuth
* clarify right handed rotation of side slope in example, also replace EG with "For example"
* clarify comment defining the tracker coord-sys

Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
* use cross_axis_slope instead of side_slope
* use calc_cross_axis_tilt() instead of calc_system_side_slope()
* use slope azimuth & tilt instead of system az & ze
* clarify cross-axis tilt is measured in the tracker reference frame
* add cross-axis tilt to the SingleAxisTracker repr()
* add see also to SingleAxisTracker
* remove Lorenzo reference
@mikofski
Copy link
Member Author

mikofski commented Sep 4, 2020

OK, refactored names, clarified argument docstrings, and removed Lorenzo reference - hopefully you will all approve. Thanks.

  • "new" ← "old"
  • slope_azimuthsystem_azimuth
  • slope_tiltsystem_zenith
  • cross_axis_tiltside_slope
  • calc_cross_axis_tiltsystem_side_slope
  • the "slope" ← "system plane"

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.

Almost there I think. This is a big lift and a significant improvement over the old tracker calculation.

docs/sphinx/source/whatsnew/v0.8.0.rst Outdated Show resolved Hide resolved
pvlib/tracking.py Outdated Show resolved Hide resolved
pvlib/tracking.py Show resolved Hide resolved
pvlib/tracking.py Outdated Show resolved Hide resolved
pvlib/tracking.py Show resolved Hide resolved
pvlib/tracking.py Outdated Show resolved Hide resolved
pvlib/tracking.py Show resolved Hide resolved
pvlib/tracking.py Outdated Show resolved Hide resolved
pvlib/tracking.py Outdated Show resolved Hide resolved
pvlib/tracking.py Outdated Show resolved Hide resolved
* clarify cross-axis tilt by using consistent wording
* use "tracker axes" when referring to tracker axes plane (or slope)
  instead of just "slope"
* update SingleAxisTracker headline to refer user to singleaxis function
* remove redundant wording in cross_axis_tilt arg, regarding tracker axes
  plane, and make it verbatim between SingleAxisTracker, singleaxis, and
  calc_cross_axis_tilt
* improve links to module index in LocalizedSingleAxisTracker
* Improve docstring for singleaxis to be explicit about right-handed
  coordinate system of tracker reference frame and right-handed
  rotation for tracker_theta
* suggested changes to comments and do multiplication before division
* change slope_tilt to be relative to horizontal rather than defined by
  normal, also use "tracker axes plane"
@mikofski
Copy link
Member Author

mikofski commented Sep 4, 2020

Hi All, thanks for your excellent comments and suggested edits. Where I've differed, my justification may have been hidden by resolving the conversation. Specifically here, here, and here. I want to restate my opinion that describing geometries and spatial orientations and rotations with words is very abstract, and IMHO there is no wording that will satisfy everyone b/c we all perceive them differently.

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.

LGTM. Thanks x10 to @mikofski for driving the bus and @cwhanse for the thorough review on both this PR and the original report.

I will put together a PR with a gallery example with some visuals to help explain the angles. Might not be ready in time for 0.8 though.

@mikofski
Copy link
Member Author

mikofski commented Sep 5, 2020

@kanderso-nrel wanna do the honors?

@kandersolar kandersolar merged commit 7fc595a into pvlib:master Sep 5, 2020
@wholmgren
Copy link
Member

Thanks @mikofski @kanderso-nrel and @cwhanse for this massive effort!

@kanderso-nrel consider removing tracking.ipynb when you write the gallery example.

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

5 participants