Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add
BaseBodycentricRepresentation
#14851Add
BaseBodycentricRepresentation
#14851Changes from 1 commit
918a6b3
8eda98e
6a5ad91
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Nitpick: might as well leave this out for now.
Instead, I think here is the place to mention how lon, lat, and height are defined relative to the ellipsoid.
Alternatively, you could clarify the definition in
geodetic_base_doc
above (and then not use it forbodycentric
)Check warning on line 103 in astropy/coordinates/representation/geodetic.py
Codecov / codecov/patch
astropy/coordinates/representation/geodetic.py#L103
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.
Nitpick, but could you get this back to the default format? Be sure to delete the final
,
aftercopy=False
, otherwiseblack
will break it over several lines again.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.
Again, I'd remove this for now and instead describe how the parameters are used (and how it is different from geodetic).
Alternatively, do not use
geodetic_base_doc
, but instead write a complete docstring here that also describes howlat
is defined.Check warning on line 128 in astropy/coordinates/representation/geodetic.py
Codecov / codecov/patch
astropy/coordinates/representation/geodetic.py#L128
Check warning on line 135 in astropy/coordinates/representation/geodetic.py
Codecov / codecov/patch
astropy/coordinates/representation/geodetic.py#L134-L135
Check warning on line 139 in astropy/coordinates/representation/geodetic.py
Codecov / codecov/patch
astropy/coordinates/representation/geodetic.py#L137-L139
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.
These variables are unused.
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.
Indeed! They are no longer needed as the calculation is done for
r
already.Check warning on line 155 in astropy/coordinates/representation/geodetic.py
Codecov / codecov/patch
astropy/coordinates/representation/geodetic.py#L148-L155
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.
Could use
np.hypot()
here:Check warning on line 163 in astropy/coordinates/representation/geodetic.py
Codecov / codecov/patch
astropy/coordinates/representation/geodetic.py#L160-L163
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.
Total nitpick, but outer parentheses are not necessary.
Check warning on line 180 in astropy/coordinates/representation/geodetic.py
Codecov / codecov/patch
astropy/coordinates/representation/geodetic.py#L172-L180
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.
Again, could you bring this on a single line?
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.
Doesn't this fit on one line?
If it does then the other similar cases should be updated analogously.
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.
Let's remove the "spanning from 0 to 360 degrees" here too.
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 didn't mean to have this part about the internal use to be removed!
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.
Maybe combine with last line while you are making the last changes.
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.
Sorry I'm not sure to understand this comment....
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.
Meant to combine the two lines to give```
Earth ellipsoids used in
~astropy.coordinates.EarthLocation
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.
bodicentric -> bodycentric
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.
originated at -> relative to
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.
"the height is the distance from the spheroid surface on the line of sight" -> "height is the distance from the surface (measured radially)."
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.
Maybe make plural: "Representations"