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

Fixed missing transformation errors when obstime is None #4267

Merged
merged 3 commits into from
Jun 12, 2020

Conversation

ayshih
Copy link
Member

@ayshih ayshih commented Jun 8, 2020

The transformations to/from SunPy 1.0 frames are fairly permissive when they encounter obstime=None for a frame and try to transfer it from the other frame in the transformation, or even the observer attribute if it's an observer-based frame. The behavior is no longer permissive if attempting to transform beyond that subgraph. (The permissive behavior was primarily motivated by our observer frame attribute, but the specific bug there has been fixed by #4266, and the concept of under-defined SkyCoord-less transformations may be going away in the future. This PR leaves this permissive behavior untouched.)

The transformations to/from SunPy 1.1 frames (HEE, GSE, GEI, HCI) don't fully handle all situations with obstime=None, and weren't raising transformation errors. Some of the transformation output in edge cases could be outright wrong.

This PR adds a bunch of transformation errors.

@ayshih ayshih added [BugFix] coordinates Affects the coordinates submodule labels Jun 8, 2020
@ayshih ayshih force-pushed the obstime_none branch 2 times, most recently from caad6c0 to cd9ec45 Compare June 8, 2020 17:13
@ayshih ayshih marked this pull request as ready for review June 8, 2020 17:14
@ayshih ayshih requested a review from a team as a code owner June 8, 2020 17:14
@nabobalis nabobalis added this to the 2.0 milestone Jun 10, 2020
Copy link
Member

@Cadair Cadair left a comment

Choose a reason for hiding this comment

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

I don't fully understand why we need these checks and not some others, or why the errors aren't already being raised in other places.

What is the difference between these transforms and the ones where we copy from the target frame?

@ayshih
Copy link
Member Author

ayshih commented Jun 11, 2020

The idea is to always have the check when transforming between an Astropy frame and a SunPy frame, for when our frame has obstime=None, because copying may be weird. The check already exists for the HCRS<->HGS transformations, so we need analogous checks for the HME<->HEE and HME<->GEI transformations.

The HGS<->HCI checks are a bit different: it'll only error when obstime=None on both ends, so the transformations are not defined.

The HEE<->GSE behavior should actually be analogous to HGS<->HCI, so I'll fix that.

@ayshih
Copy link
Member Author

ayshih commented Jun 11, 2020

Okay, HEE<->GSE transformations are now fixed. Plus, I realized that there were unnecessary computations in those transformations, so I also simplified them.

@Cadair Cadair added the Merge When CI Passes Hit that merge button when it's all green! label Jun 12, 2020
@Cadair Cadair merged commit e4c5fda into sunpy:master Jun 12, 2020
@ayshih ayshih deleted the obstime_none branch August 20, 2020 04:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coordinates Affects the coordinates submodule Merge When CI Passes Hit that merge button when it's all green!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants