[BUGFIX] Inconsistent naming of `[cross_]axis_tilt` / `[cross_]axis_slope`
- [x] Closes #2334
- [x] I am familiar with the contributing guidelines
- [x] Tests added
- [NA] Updates entries in
docs/sphinx/source/referencefor API changes. - [x] Adds description and name entries in the appropriate "what's new" file in
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`). - [NA] New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
- [x] Pull request is nearly complete and ready for detailed review.
- [x] Maintainer: Appropriate GitHub Labels (including
remote-data) and Milestone are assigned to the Pull Request and linked Issue.
pvlib.shading.shaded_fraction1d uses 'cross_axis_slope' as an input, while pvlib.tracking.singleaxis uses 'cross_axis_tilt'. I think these are the same thing - is that correct? If so, would it make sense to update one of them to be consistent?
Extract from the body message, OP @williamhobbs, #2334 .
Also adds variable description to nomenclature page.
~Relevant doc pages (old)~
- ~https://pvlib-python--2543.org.readthedocs.build/en/2543/user_guide/extras/nomenclature.html#term-cross_axis_tilt~
- ~https://pvlib-python--2543.org.readthedocs.build/en/2543/reference/generated/pvlib.shading.shaded_fraction1d.html~
- ~https://pvlib-python--2543.org.readthedocs.build/en/2543/reference/generated/pvlib.tracking.singleaxis.html~
Relevant doc pages (new)
- https://pvlib-python--2543.org.readthedocs.build/en/2543/whatsnew.html
- https://pvlib-python--2543.org.readthedocs.build/en/2543/reference/generated/pvlib.shading.shaded_fraction1d.htm
- https://pvlib-python--2543.org.readthedocs.build/en/2543/reference/generated/pvlib.shading.projected_solar_zenith_angle.html
- https://pvlib-python--2543.org.readthedocs.build/en/2543/reference/generated/pvlib.tracking.singleaxis.html
- https://pvlib-python--2543.org.readthedocs.build/en/2543/reference/generated/pvlib.tracking.calc_surface_orientation.html
- https://pvlib-python--2543.org.readthedocs.build/en/2543/reference/generated/pvlib.tracking.calc_axis_slope.html
- https://pvlib-python--2543.org.readthedocs.build/en/2543/reference/generated/pvlib.tracking.calc_cross_axis_slope.html
- https://pvlib-python--2543.org.readthedocs.build/en/2543/reference/generated/pvlib.pvsystem.SingleAxisTrackerMount.html
- https://pvlib-python--2543.org.readthedocs.build/en/2543/user_guide/extras/nomenclature.html#term-cross_axis_slope
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.
The original thread #2334 contained multiple views in favor of cross_axis_slope over cross_axis_tilt. This PR currently implements the latter. Is there a reason for this?
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.
After a deeper review on the required changes to port cross_axis_tilt to cross_axis_slope, note that we don't only have what was discussed in #2334:
-
cross_axis_tiltas parameter inpvlib.tracking.singleaxis
But some other occurrences that could come along with the same renaming:
- Function name/signature
pvlib.tracking.calc_axis_tilt - Function name/signature
pvlib.tracking.calc_cross_axis_tilt - Dataclass field
cross_axis_tiltinpvlib.pvsystem.SingleAxisTrackerMount
The other way around is what is currently implemented in this PR, cross_axis_slope to cross_axis_tilt:
-
cross_axis_slopeas parameter inpvlib.shading.shaded_fraction1d
I personally would prefer coherence among all of them. Either tilts or slopes. An example of what I mean, if I only address 1., singleaxis would take cross_axis_slope, that would be calculated with calc_cross_axis_tilt. Feels weird to me.
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 __init__, then decorating it with the renamed kwarg util would work.
To sum, my points:
a. To which extent do we want to rename *_axis_tilts into *_axis_slopes? Regarding tracker axes on the terrain.
b. Should we abide to the known precedent?
c. The number of deprecations, if we push towards all of them, that's 1 vs 4.
My two opinionated cents:
a. I lean towards consistency, i.e., renaming either all or none.
b. It's not like tilt is wrong, it's looks to me it's just a nuance between the options, and to distinguish it from surface tilt [0, 90]º.
c. As long as my opinionated b. is not flawed, I suggest prioritising user experience.
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, https://github.com/pvlib/pvlib-python/discussions/2334#discussioncomment-14477715, 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"...
Hella lot of changes in the new diff. My condolences to reviewers. For the docs, you may want to start reviewing at the whatsnew: https://pvlib-python--2543.org.readthedocs.build/en/2543/whatsnew.html
@echedey-ls fyi -- it seems like this change needs more discussion and review before it's ready for merge, so I moved the milestone to v0.13.2.
No problem!
Adding an illustration to the docstring of pvlib.tracking.singleaxis would help greatly to explain axis_slope and cross_axis_slope.
Adding an illustration to the docstring of
pvlib.tracking.singleaxiswould help greatly to explainaxis_slopeandcross_axis_slope.
Agreed. I suggest a new issue/PR for that. IMO, diff is already big in this PR + personally I don't have time to contribute now.
Adding an illustration to the docstring of
pvlib.tracking.singleaxiswould help greatly to explainaxis_slopeandcross_axis_slope.
Agreed. I suggest a new issue/PR for that. IMO, diff is already big in this PR + personally I don't have time to contribute now.
Are there existing examples of illustrations in docstrings? https://pvlib-python.readthedocs.io/en/latest/reference/generated/pvlib.shading.shaded_fraction1d.html is the only one I know of.
A collection of diagrams/illustrations for pvlib with common design themes/standards and a straightforward way to update them would be really cool. I know I've talked with @kandersolar and @mikofski about tools to create diagrams (other than PowerPoint, my unfortunate go-to), but I can't remember what they use.
IK it's not planned for this milestone, but just in case, I've updated the version it was supposed to be deprecated to this release.