-
-
Notifications
You must be signed in to change notification settings - Fork 276
Add cartesian_to_spherical(). #1365
base: main
Are you sure you want to change the base?
Conversation
656ac1c
to
9faeab7
Compare
Codecov Report
@@ Coverage Diff @@
## main #1365 +/- ##
==========================================
+ Coverage 92.08% 92.38% +0.30%
==========================================
Files 82 82
Lines 4346 4283 -63
Branches 363 357 -6
==========================================
- Hits 4002 3957 -45
+ Misses 255 246 -9
+ Partials 89 80 -9
Continue to review full report at Codecov.
|
64bc553
to
4a7b701
Compare
@astrojuanlu Something seems wrong with the checks. Not sure why they fail. |
sph_car(np.array([0.5, np.pi / 4, -np.pi / 4]), np.array([0.25, -0.25, 0.35355339])) | ||
|
||
sph_car( | ||
np.array([0.5, -np.pi / 4, np.pi / 4]), np.array([-0.25, -0.25, 0.35355339]) | ||
) | ||
|
||
result = spherical_to_cartesian(np.array([0.5, -np.pi / 4, np.pi / 4])) | ||
expected = np.array([-0.25, -0.25, 0.35355339]) | ||
assert np.allclose(expected, result) | ||
sph_car( | ||
np.array([[0.5, np.pi / 4, -np.pi / 4], [0.5, -np.pi / 4, np.pi / 4]]), | ||
np.array([[0.25, -0.25, 0.35355339], [-0.25, -0.25, 0.35355339]]), | ||
) | ||
|
||
result = spherical_to_cartesian( | ||
np.array([[0.5, np.pi / 4, -np.pi / 4], [0.5, -np.pi / 4, np.pi / 4]]) | ||
sph_car( | ||
np.array([2.60564963, 1.75305207, 4.4458828]), | ||
np.array([-0.674864797187, -2.472029259161, -0.472269863940]), |
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 suggest we use pytest.mark.parametrize
for these, what do you think?
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 have no experience with pytest so I cannot give an opinion. But these particular values were chosen because they trigger corner cases due to the use of arctan2 and arccos. There are probably nicer ways to test the corner cases but I didn't have much time to think about them.
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.
In fact, if there was a way to sample the full range of values in terms of positive/negative directions, that would be even better.
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.
Right! I'll have a look at this and try a version with Hypothesis + pytest then
c77ef79
to
1791334
Compare
No description provided.