Fix operator precedence in climb_angle - #103
Merged
dbetchkal merged 1 commit intoJul 24, 2026
Merged
Conversation
The arcsin argument was written as np.dot(n, v) / np.linalg.norm(n) * np.linalg.norm(v), which evaluates as (n.v / |n|) * |v| rather than n.v / (|n| * |v|). Since n is a unit vector this reduces to arcsin(v_z * |v|), so any vector longer than unit length pushes the argument past 1 and numpy returns NaN. The docstring formula and the implementation in legacy_code/active_space_utils.py both use the parenthesized form. Added tests. Signed-off-by: arpitjain099 <arpitjain099@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Noticed this reading through
computation.py.climb_angledoesn't match its own docstring:Python evaluates that left to right, so it works out to
(n.v / |n|) * |v|instead ofn.v / (|n| * |v|). The docstring givesA = n.b = |n||b|sin(θ), and the version inlegacy_code/active_space_utils.pyhas the parentheses, so I think this is just a typo that slipped in during the port.Because
nis a unit vector the division by|n|is a no-op, and the code ends up computingarcsin(v_z * |v|). Anything longer than unit length pushes the arcsin argument past 1, so numpy returns NaN:[3, 0, 4]returns NaN, should be 53.13 degrees[1, 1, 1]returns NaN, should be 35.26[10, 0, 10]returns NaN, should be 45It only gives the right answer when
|v|happens to be exactly 1.Worth saying up front: nothing in the package calls this today, so I can't point at any actual breakage. (The
climb_anglementions inactive_space_generator.pyare a trajectory column that happens to share the name, not this function.) It is exported in__all__though, so it's reachable as public API, and the math is wrong either way.I put the tests in their own file instead of
tests/utils/test_computation.pysince #95 is currently adding that file and I didn't want the two to collide. Happy to fold them in there once that one lands.