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
Update handling of solar radius in sunpy.map #5416
Conversation
changelog/xxxx.feature.rst
Outdated
@@ -0,0 +1,3 @@ | |||
`sunpy.map.GenericMap.rsun_meters` now uses `sunpy.map.GenericMap.rsun_obs` | |||
as a fallback to calculate the radial distance if RSUN_REF metadata isn't |
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.
"radial distance" is potentially confusing, since a reader may think you mean the Sun-observer distance (which is the radius
in HeliographicStonyhurst
).
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 also changed the property docstring slightly, and went with "assumed radius of observed emission". Other suggestions welcome.
sunpy/coordinates/sun.py
Outdated
@@ -70,6 +70,11 @@ def _angular_radius(sol_radius, distance): | |||
return Angle(solar_semidiameter_rad.to(u.arcsec)) | |||
|
|||
|
|||
@u.quantity_input | |||
def _radius(angular_radius: u.arcsec, distance: u.m): |
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 suggest being more verbose in the name of this function, e.g., _physical_radius_from_angular_radius()
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 went with _radius_from_angular_radius
, but happy to go more explicit again and put physical
at the front.
9fbbe5b
to
360ad40
Compare
70679fd
to
b7306d1
Compare
Hello @dstansby! Thanks for updating this PR.
Comment last updated at 2021-07-10 11:33:09 UTC |
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.
Minor wording nitpicks
This cleans up handling of
rsun_obs
andrsun_meters
so:rsun_meters
.rsun_meters
can now rely onrsun_obs
metadata to calculate a value, if presentwarning
tolog.debug
? #5285