Skip to content
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

Provide a planar screen as an optional assumption for handling off-disk helioprojective coordinates #7115

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

ayshih
Copy link
Member

@ayshih ayshih commented Jul 26, 2023

Expanding beyond assume_spherical_screen() (see #4003), this PR adds a planar screen as an optional assumption for handling off-disk helioprojective coordinates.

screen_comparison

See also #6411

@ayshih ayshih added coordinates Affects the coordinates submodule No Backport A PR that isn't to be backported to any release branch. (To be used as a flag to other maintainers) labels Jul 26, 2023
@wtbarnes wtbarnes added this to the 5.1.0 milestone Oct 4, 2023
@dstansby dstansby added the Whats New? Needs a section added to the current Whats New? page. label Oct 18, 2023
@dstansby dstansby removed this from the 5.1.0 milestone Oct 24, 2023
@github-actions github-actions bot added the Stale The bot will close this PR after 6 months label Mar 23, 2024
@nabobalis nabobalis removed the Stale The bot will close this PR after 6 months label Mar 28, 2024
@sunpy sunpy deleted a comment from github-actions bot Mar 28, 2024
@wtbarnes
Copy link
Member

wtbarnes commented Mar 29, 2024

OK I've taken a crack at implementing the screens as separate classes and modifying the context manager and make_3d such that they can handle any of these screen classes and all of the logic of those screen assumptions is in those particular classes.

Some things to still left to do/decide

  • Is this the API we want? Should we instead make the classes themselves the context managers? If the latter, I'm not sure how to handle modifying the class attribute _assumed_screen in the case where the context manager is not a class method
  • Implement the context manager on the classes themselves
  • Make BaseScreen an abstract base class
  • assume_spherical_screen should be deprecated in favor of assume_screen
  • Docstrings need to be updated
  • Examples need to be updated
  • Modify PlanarScreen to allow for a variable distance from the center of the Sun
  • Changelog
  • Understand what happens when you're looking away from the planar screen
  • Replace assume_spherical_screen with SphericalScreen everywhere

@wtbarnes
Copy link
Member

wtbarnes commented Apr 5, 2024

How this now works:

import astropy.units as u
from astropy.coordinates import SkyCoord

from sunpy.coordinates import Helioprojective, SphericalScreen, PlanarScreen

frame = Helioprojective(observer='earth', obstime='2020-01-01')
coord = SkyCoord(Tx=[-1000, 300, 1000]*u.arcsec, Ty=[-1000, 300, 1000]*u.arcsec, frame=frame)

with SphericalScreen(coord.observer):
    _ = coord.make_3d()

with PlanarScreen(coord.observer):
    _ = coord.make_3d()

with PlanarScreen(coord.observer, distance_from_center=1*u.Rsun):
    _ = coord.make_3d()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coordinates Affects the coordinates submodule No Backport A PR that isn't to be backported to any release branch. (To be used as a flag to other maintainers) Whats New? Needs a section added to the current Whats New? page.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants