Skip to content
This repository has been archived by the owner on Oct 14, 2023. It is now read-only.

Expand numpy in docstring #1483

Merged
merged 2 commits into from Feb 16, 2022
Merged

Conversation

jerobado
Copy link
Contributor

Fixes #1482

@Yash-10
Copy link
Member

Yash-10 commented Feb 15, 2022

Thanks, @jerobado for this pull request!

While this solves one case, I see a few more cases in the docstrings where np.array is used instead of numpy.array, which can be seen by searching for "np.array" in the top-left corner in GitHub, for example. Could you also fix those?

@Yash-10
Copy link
Member

Yash-10 commented Feb 15, 2022

Also, I think numpy.ndarray would be a better choice than numpy.array.

@jerobado
Copy link
Contributor Author

Got it @Yash-10. I'll search for other cases. Thank you!

@jerobado
Copy link
Contributor Author

Hello @Yash-10, I was able to find these additional files that needs to updated:

src/poliastro/core/elements.py
src/poliastro/core/util.py
src/poliastro/maneuver.py

Can you kindly confirm if these two lines needs to be included as well?

@Yash-10
Copy link
Member

Yash-10 commented Feb 16, 2022

Can you kindly confirm if these two lines needs to be included as well?

Great! Yes, they would also need to be changed to numpy.ndarray since just ndarray does not seem to help documentation identify it correctly.

Apart from this, I also realized that these files:

src/poliastro/core/thrust/change_a_inc.py
src/poliastro/core/thrust/change_argp.py
src/poliastro/core/iod.py

also have docstrings that show numpy.array which also would be a good thing to change to numpy.ndarray

@Yash-10
Copy link
Member

Yash-10 commented Feb 16, 2022

The documentation build error seems unrelated (not sure, though, about its cause) but the changes look good to me!

Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@astrojuanlu astrojuanlu merged commit 38a28e4 into poliastro:main Feb 16, 2022
@astrojuanlu
Copy link
Member

Forcing a merge of this one, as @Yash-10 says the docs error seems unrelated

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

numpy.ndarray instead of np.array in docstrings?
3 participants