ManimCommunity/manim

Confusing/Misleading docstring for cartesian_to_spherical

Open

#3123 opened on Jan 15, 2023

View on GitHub
 (2 comments) (0 reactions) (0 assignees)Python (17,820 stars) (1,378 forks)batch import
good first issue

Description

I was using cartesian_to_spherical in space_ops.py but got confused by the docstring. In there it says it returns (distance, phi, theta) however what the function calls phi and theta is not returned in that order. Because both cartesian_to_spherical and spherical_to_cartesian use this order one can convert back and forth. See https://github.com/ManimCommunity/manim/blob/main/manim/utils/space_ops.py#L736-L776 .

However I think the order is confusing, or at least inconsistent with things like the value trackers of ThreeDCamera.

I'm not sure what the way to tackle issues like this is so although I initially wanted to submit a PR fixing the docstring I don't know if that is even correct anymore.

Contributor guide

Tech stack
python
Domain
documentation
Issue type
documentation
DifficultyEstimated implementation difficulty for a new contributor, from 1 for very small changes to 5 for expert-level work.
2
Estimated timeA rough time range for an experienced contributor to investigate, implement, test, and prepare a pull request.
under 1 hour
Activity statusHow available the issue appears right now: fresh, active, stale, blocked, or waiting on maintainer input.
stale
ClarityHow clearly the issue explains the expected change, acceptance criteria, and next step.
clear
Prerequisites
basic Pythonunderstanding of coordinate systems
Newbie friendlinessA 1-100 score estimating how approachable this issue is for first-time contributors.
85
Research direction
The issue references the function in manim/utils/space ops.py (lines 736-776) and notes inconsistency with ThreeDCamera's value trackers in manim/camera/three d camera.py (line 87). First, read the docstring and implementation of cartesian to spherical to understand the current return order. Then, compare with the usage in three d camera.py to determine the expected order. Finally, update the docstring to accurately describe the return order, or if a deeper fix is needed, discuss in the issue before making changes.