Fix calc_surface_orientation broadcasting for >1-D tracker_theta#2749
Fix calc_surface_orientation broadcasting for >1-D tracker_theta#2749ArthurOnnoTerabase wants to merge 2 commits into
Conversation
The unit-normal-based implementation introduced in v0.15.1 (pvlib#2702) used np.column_stack inside _unit_normal, which only handles 1-D inputs correctly. For 2-D (or higher rank) tracker_theta the column-stacked array had the wrong shape, and the subsequent np.where in calc_surface_orientation raised a ValueError. Switch to np.stack(..., axis=-1) and use ellipsis indexing in calc_surface_orientation so the function preserves the input rank for arbitrary-shape tracker_theta. np.atleast_1d preserves the historical (1, 3) return shape of _unit_normal for scalar input. Also adds a parametrised regression test that exercises 2-D and 3-D tracker_theta inputs. Closes pvlib#2747 Co-authored-by: Cursor <cursoragent@cursor.com>
ArthurOnnoTerabase
force-pushed
the
fix/calc-surface-orientation-2d-broadcast
branch
from
2969cbd to
9f7cf0d
Compare
kandersolar
left a comment
kandersolar
left a comment
There was a problem hiding this comment.
Great PR @ArthurOnnoTerabase, thanks! One request below.
| rng = np.random.default_rng(0) | ||
| rotations_flat = rng.uniform(-90, 90, size=int(np.prod(shape))) |
There was a problem hiding this comment.
This will be the first and only (I think) random number generation in pvlib's test suite. I lean towards not using random numbers for (1) fear of them silently changing on some new numpy and (2) opaque serendipitous effects. Of course the values themselves should not matter, but in principle I think ensuring stability is a good idea.
Could we get rid of the RNG and use rotations_flat = np.linspace(-90, 90, size=int(np.prod(shape))) instead?
There was a problem hiding this comment.
Fair point. Done! Update was tested locally before pushing the new commit, worked as expected (the test still reproduces the broadcasting ValueError on the unfixed code).
Indeed, I asked Cursor to check and it could not find any other random number generator in the suite. Shall the Contributing documentation include a short mention about refraining from using random generators?
There was a problem hiding this comment.
I don't think that note is needed.
Random not advisable, deterministic inputs better in tests
3 participants
The unit-normal-based implementation introduced in v0.15.1 (#2702) used np.column_stack inside _unit_normal, which only handles 1-D inputs correctly. For 2-D (or higher rank) tracker_theta the column-stacked array had the wrong shape, and the subsequent np.where in calc_surface_orientation raised a ValueError.
Switch to np.stack(..., axis=-1) and use ellipsis indexing in calc_surface_orientation so the function preserves the input rank for arbitrary-shape tracker_theta. np.atleast_1d preserves the historical (1, 3) return shape of _unit_normal for scalar input.
Also adds a parametrised regression test that exercises 2-D and 3-D tracker_theta inputs.
Closes #2747
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.