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

Some docstring changes and an error check #1264

Merged

Conversation

Yash-10
Copy link
Member

@Yash-10 Yash-10 commented Jun 27, 2021

The core propagation docstrings mentioned astropy units instead of floats.

The get_mean_elements accessed the name property without checking for the body. As a result, the logic didn't seem to go the except block and instead raised an in-built python error if None was passed as the body, for example. Would having a custom error help in such a case?

Thanks!

@Yash-10
Copy link
Member Author

Yash-10 commented Jun 27, 2021

@Yash-10 Yash-10 force-pushed the Some-test-additions-or-modifications branch from 6ea7a5c to 139d63a Compare June 27, 2021 13:03
@codecov
Copy link

codecov bot commented Jun 27, 2021

Codecov Report

Merging #1264 (0b32a2b) into main (37947f3) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1264      +/-   ##
==========================================
+ Coverage   91.67%   91.76%   +0.08%     
==========================================
  Files          79       79              
  Lines        4157     4153       -4     
  Branches      364      362       -2     
==========================================
  Hits         3811     3811              
+ Misses        262      259       -3     
+ Partials       84       83       -1     
Impacted Files Coverage Δ
src/poliastro/core/propagation/__init__.py 96.00% <ø> (ø)
src/poliastro/core/propagation/farnocchia.py 79.45% <ø> (ø)
src/poliastro/core/spheroid_location.py 100.00% <ø> (ø)
src/poliastro/spheroid_location.py 100.00% <ø> (ø)
src/poliastro/twobody/propagation.py 91.48% <ø> (ø)
src/poliastro/bodies.py 85.18% <100.00%> (ø)
src/poliastro/core/iod.py 91.16% <100.00%> (-0.10%) ⬇️
src/poliastro/frames/util.py 89.47% <100.00%> (ø)
src/poliastro/maneuver.py 97.70% <100.00%> (+2.19%) ⬆️
src/poliastro/twobody/mean_elements.py 100.00% <100.00%> (+11.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37947f3...0b32a2b. Read the comment docs.

@jorgepiloto
Copy link
Member

@Yash-10 Yash-10 force-pushed the Some-test-additions-or-modifications branch from 1ac5bff to 2d33e26 Compare June 28, 2021 09:52
@astrojuanlu
Copy link
Member

Let's better use this link for the Pimienta propagator: http://ancs.eng.buffalo.edu/pdf/ancs_papers/2012/ProtoType.pdf

@@ -23,6 +25,10 @@ def get_mean_elements(body, epoch=J2000):
# So we are reverting the conversion in the last line
# This way at least we avoid copy pasting the values
try:
if body not in SOLAR_SYSTEM_BODIES:
Copy link
Member

Choose a reason for hiding this comment

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

I just realized that Sun is an instance of SolarSystemPlanet... I wonder if we should change that somehow 😅

Copy link
Member

Choose a reason for hiding this comment

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

In any case, what's the point of checking this list first? The line

body_index = PLAN94_BODY_NAME_TO_PLANET_INDEX[name]

tells me that we could replace this check by

if name not in PLAN94_BODY_NAME_...

and so we wouldn't need SOLAR_SYSTEM_BODIES. Right?

Copy link
Member Author

@Yash-10 Yash-10 Jul 2, 2021

Choose a reason for hiding this comment

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

I recall the reason was to handle if someone inputs body=None or any other invalid body such that

name = body.name.lower()

would fail in the next line, since then we would get the error:

AttributeError: 'NoneType' object has no attribute 'name'

for example.

So would it be good to check for body first, instead of name?

Copy link
Member

Choose a reason for hiding this comment

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

I think we shouldn't shield against every possible misuse of the API. The documentation and the signature are very clear:

def get_mean_elements(body, epoch=J2000):
"""Get ecliptic mean elements of body.
Parameters
----------
body : ~poliastro.bodies.SolarSystemPlanet
Body.

If the user passes None as a body... AttributeError: 'NoneType' object has no attribute 'name' is a perfect explanation of what happened :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for this suggestion!

Now, it seems that there might be no need of checking if name not in ... since it's already inside a try-except block? Instead we could add to the existing error message stating that it must be an instance of SolarSystemPlanet and change Sun to not be an instance of SolarSystemPlanet?

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.

Left a few comments

@Yash-10 Yash-10 force-pushed the Some-test-additions-or-modifications branch from 476f705 to 907aadd Compare July 2, 2021 06:14
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.

Looks good to me except for the citation, but not a big deal 👍🏽

@Yash-10
Copy link
Member Author

Yash-10 commented Jul 2, 2021

I tried to fix the type hint failures.

@jorgepiloto
Copy link
Member

Merging, thanks @Yash-10 👍🏽

@jorgepiloto jorgepiloto merged commit 1988006 into poliastro:main Jul 4, 2021
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.

None yet

3 participants