-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[BUGFIX] Inconsistent naming of cross_axis_tilt
/ cross_axis_slope
#2543
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
base: main
Are you sure you want to change the base?
[BUGFIX] Inconsistent naming of cross_axis_tilt
/ cross_axis_slope
#2543
Conversation
Currently targets v0.13.1, but I leave this decision up to anybody else (please, assign milestone accordingly). I will update deprecations tracker when merged. |
since="0.13.1", | ||
old_param_name="cross_axis_slope", | ||
new_param_name="cross_axis_tilt", | ||
removal="0.15.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removal="0.15.0", |
We usually don't specify a removal version as there's not standard timeline for when minor versions come out. If we really wanted to specify a removal, I think this should be in the form of a date, e.g., removal="Earliest September 2026"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I never thought of that, but looks a promising idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm interested in what @wholmgren thinks of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed this for a few weeks. I don't think timeline is particularly relevant so long as we are using SemVer (or something close to it). I personally think we should specify removal versions, they should default to the 0.(m+2).0 release, and we should use our existing test machinery to enforce that. If we had several minor releases in a very short period of time (a few weeks) then I'd be ok pushing out the removal to a future release.
The original thread #2334 contained multiple views in favor of |
Thanks for the reviews. @AdamRJensen , yeah, my fault. This is a stale branch I had locally - I guess I took that decision to conform with what already was the term prior to the newer feature and just skimmed through the discussion. I will change this whenever I can to reflect what was discussed. |
Co-Authored-By: RDaxini <[email protected]>
After a deeper review on the required changes to port
But some other occurrences that could come along with the same renaming:
The other way around is what is currently implemented in this PR,
I personally would prefer coherence among all of them. Either tilts or slopes. An example of what I mean, if I only address 1., Note 1. and 5. are solvable with the renamed kwarg warning util; 2. and 3. by copying the new signature into an old reference variable, prior decoration with the deprecated util. À la: # allow deprecated name from calc_cross_axis_slope
# (original name was calc_cross_axis_tilt)
calc_cross_axis_tilt = deprecated(
since="0.13.1", alternative="pvlib.tracking.calc_cross_axis_slope"
)(calc_cross_axis_slope) Regarding 4., I have my doubts. Thou probably populating and overriding its To sum, my points: a. To which extent do we want to rename My two opinionated cents: Nonetheless, a weak opinion. Please leave your opinion and suggestions. I will do as requested. CC: @pvlib/pvlib-maintainer |
I believe we selected “tilt” versus slope because we wanted to distinguish between the “tilt” of the plane containing tracker axes versus the slope of the underlying terrain which might differ and is not relevant to the tracker rotation calculation. IMO slope has a connotation that implies terrain. |
I added a comment in the issue, #2334 (reply in thread), but I'll duplicate it here: I'll caveat this with noting that I don't think it matters too much, but I still prefer "slope." For example, I can mention "slope-aware backtracking," and it is relatively unambiguous what I am talking about. If I were instead to mention "tilt-aware backtracking," I think the meaning is lost. And I informally asked a few non-solar folks in the office about the image below. Everyone said the angle of B should be "tilt" and C should be "slope" or "incline." When I said B has to be "tilt" and asked if A should be "tilt" or "slope", everyone liked "slope" more and said that "tilt" would be confusing. One person did mention that slope makes them think of "rise over run"... ![]() |
docs/sphinx/source/reference
for API changes.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`
).remote-data
) and Milestone are assigned to the Pull Request and linked Issue.Extract from the body message, OP @williamhobbs, #2334 .
Also adds variable description to nomenclature page.
Relevant doc pages