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
Fixed order of return values of :func:.space_ops.cartesian_to_spherical
#2168
Conversation
Thanks for fixing this! It seems like that a specific test is failing now, can you look into that? Feel free to ask if you need help. |
Woops forgot to check if tests went good after my commit, forgot they were added in #2095. |
cartesian_to_spherical
cartesian_to_spherical
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.
Great! LGTM
This is just a general remark: what do you think about using named tuples in cases like these? |
Could be a good idea to avoid having situations like this one where the tuple order was incorrect, but we wouldn't be able to take advantage of the numpy arrays operations. |
I wonder whether this would really be so much of a drawback. But maybe we'll come back to it eventually. In any case, thank you for your efforts! |
cartesian_to_spherical
.space_ops.cartesian_to_spherical
Overview: What does this pull request change?
As mentioned in #2095, the returned array in
cartesian_to_spherical
is not in the correct order.spherical_to_cartesian
is unpacking them correctly asr, theta, phi
butcartesian_to_spherical
returns the array in that order:r, phi, theta
.position_tip
was also edited to be in accordance with the correct order.Motivation and Explanation: Why and how do your changes improve the library?
Links to added or changed documentation pages
Further Information and Comments
Reviewer Checklist