Fix for the annoying skyoffset origin bugs by mhvk · Pull Request #11730 · astropy/astropy

mhvk · 2021-05-11T20:31:44Z

It turned out there were two separate bugs: one that the origin validation cannot generally be done, since it varies by SkyOffsetFrame class. For that, I ended up just not propagating 'origin' to SkyCoord. After all, it is not like the other atttributes, which can still be used in a future transformation.

But it turned out 'origin' was also set on the SkyCoord at the end of a transformation (where for reasons we need to fix, the whole initialization gets redone even though everything is perfectly organized already). This was because of a bug in the argument parser, which led to frame attributes to be needlessly set on the SkyCoord.

@eteq - am hoping this can still be backported...

mhvk · 2021-05-11T20:37:08Z

Am a bit unsure about the milestone - was this a bug in 4.0 as well?

Also, will add a changelog entry once the tests have run.

mhvk · 2021-05-11T20:39:23Z

 from astropy.tests.helper import assert_quantity_allclose as assert_allclose


+def test_altaz_attribute_transforms():


The large diff here is just because I swapped test_altaz and test_icrs as that made the bug appear much more reliably. There is no change in code.

nstarman · 2021-05-16T04:17:53Z

+        # See gh-11277.
+        # TODO: Should it be a property of the frame attribute that it can
+        # or cannot be stored on a SkyCoord?
+        frame_kwargs.pop('origin', None)


Does this means that SkyCoord(SkyOffsetFrame(...)).origin no longer works?

#11730 (comment) Answers ↑.

Followup: given that users can make their own frames with arbitrary frame attributes, perhaps this special-casing of "origin" should be further special-cased by first checking if the 'from'-frame is SkyOffsetFrame

Suggested change

frame_kwargs.pop('origin', None)

if self.frame.__class__ is SkyOffsetFrame:

frame_kwargs.pop('origin', None)

You are right. I never liked this somewhat arbitrary popping of a name - really, SkyOffsetFrame itself should somehow tell that its attribute should not be propagated.

mhvk · 2021-05-16T13:03:06Z

@nstarman - let me take your question out of the code comments, since it is important:

Does this means that SkyCoord(SkyOffsetFrame(...)).origin no longer works?

I checked and, no, the regular usage works:

sc = SkyCoord(1*u.deg, 1*u.deg, frame=SkyOffsetFrame(origin=SkyCoord(10.*u.deg, 20*u.deg)))
SkyCoord(sc).origin
<ICRS Coordinate: (ra, dec) in deg
    (10., 20.)>

What will fail, though, is

sc_icrs = sc.icrs
sc_icrs.origin
# None

This would seem OK - there is no particular use to still know what the origin is once you have transformed away from it. The reason it is there for other attributes like location, it to make it easy to, e.g., transform back to 'altaz'. But it seems less logical that there is a way to allow one to transform back to an offset frame by passing in an explicit SkyOffsetFrame instance.

Though, this does make me realize this is an API change: what worked before but is now impossible is,

sc_icrs.transform_to('skyoffseticrs')

I think to keep that, we'll need to distinguish between different origin - in the current scheme, that means they will have to have a different name, which seems like an even bigger API break.

nstarman · 2021-05-16T15:50:54Z

I checked and, no, the regular usage works:

👍.

What will fail, though, is
sc_icrs = sc.icrs
sc_icrs.origin
This would seem OK - there is no particular use to still know what the origin is once you have transformed away from it.

Agreed. Especially since one can transform from one sky-offset to another.

Though, this does make me realize this is an API change: what worked before but is now impossible is,
sc_icrs.transform_to('skyoffseticrs')

Wow. That's dangerous and seems a good thing to deprecate. As there can be many different skyoffset frames defined, the following would be super confusing

sc1 = SkyCoord(1*u.deg, 1*u.deg, frame=SkyOffsetFrame(origin=SkyCoord(10.*u.deg, 20*u.deg)))
sc2 = SkyCoord(1*u.deg, 1*u.deg, frame=SkyOffsetFrame(origin=SkyCoord(-10.*u.deg, -20*u.deg)))

sc1_icrs = sc1.icrs
sc2_icrs = sc2.icrs

sc1_icrs.transform_to('skyoffseticrs')  # reverts to SOF1
sc2_icrs.transform_to('skyoffseticrs')  # reverts to SOF2

# or even worse
sc3 = sc1.transform_to(sc2.frame)
sc3_icrs  = sc3.icrs
sc3_icrs.transform_to('skyoffseticrs')  # it goes back to SOF2, even though it's sc1 coordinates.

I think to keep that, we'll need to distinguish between different origin - in the current scheme, that means they will have to have a different name, which seems like an even bigger API break.

Perhaps SkyOffsetFrame could have a kwarg "name" to register in frames. Unnamed SkyOffsetFrames cannot be referred to by a string.

namedframe = SkyOffsetFrame(origin=SkyCoord(10.*u.deg, 20*u.deg), name="that thing")
sc_icrs.transform_to("that thing") == sc_icrs.transform_to(namedframe)

adrn

This looks good as a bugfix! I think in the long term, as discussed by @mhvk and @nstarman, we should add support to Attribute to allow a defined attribute to tell the transform machinery not to propagate it through a frame change. For example, maybe it would be sufficient to add a propagate=False argument to Attribute?

Note: see #11782 to track the general case.

adrn · 2021-05-24T16:53:09Z

Ah, but this does need a changelog entry!

This was promised to be the case in _parse_coordinate_arg, but actually did not happen.

This causes problems with validation, since origin can be defined on different SkyOffsetFrame classes, which have different rules for what is valid. But it also makes little sense to carry it around, as it is not an attribute that can be used to set defaults for a transformation.

mhvk · 2021-05-24T17:04:21Z

Oops, now added an entry. Running tests again just in case.

adrn · 2021-05-24T17:40:11Z

Thanks @mhvk! Note @eteq - would be great to get this into 4.3 if possible (this is a bugfix)

…n-bugs Fix for the annoying skyoffset origin bugs

Fix for the annoying skyoffset origin bugs

eteq · 2021-06-11T15:46:42Z

backported to 4.3.x in 2cf2248

mhvk added Bug coordinates labels May 11, 2021

mhvk added this to the v4.3 milestone May 11, 2021

mhvk mentioned this pull request May 11, 2021

Test failure with coordinates: 'NoneType' object has no attribute 'to_geodetic' #11277

Closed

1 task

mhvk commented May 11, 2021

View reviewed changes

pllim requested review from adrn and eteq May 11, 2021 20:42

nstarman reviewed May 16, 2021

View reviewed changes

adrn approved these changes May 24, 2021

View reviewed changes

nstarman approved these changes May 24, 2021

View reviewed changes

adrn mentioned this pull request May 24, 2021

Add support for coordinate frame Attribute's to not propagate when transformed #11782

Open

mhvk added 4 commits May 24, 2021 13:03

Reverse order that brings out bug better

abaff89

Ensure that attributes from a frame are not set on SkyCoord.

77f1694

This was promised to be the case in _parse_coordinate_arg, but actually did not happen.

Changelog entry.

aeb422c

mhvk force-pushed the coordinate-skyoffset-origin-bugs branch from 1d5f5bc to aeb422c Compare May 24, 2021 17:04

adrn merged commit e9f732a into astropy:main May 24, 2021

mhvk deleted the coordinate-skyoffset-origin-bugs branch May 24, 2021 17:54

eteq pushed a commit to eteq/astropy that referenced this pull request Jun 11, 2021

Merge pull request astropy#11730 from mhvk/coordinate-skyoffset-origi…

21b8792

…n-bugs Fix for the annoying skyoffset origin bugs

eteq pushed a commit that referenced this pull request Jun 11, 2021

Merge pull request #11730 from mhvk/coordinate-skyoffset-origin-bugs

2cf2248

Fix for the annoying skyoffset origin bugs

Navigation Menu

Repository navigation

Fix for the annoying skyoffset origin bugs - #11730

Fix for the annoying skyoffset origin bugs#11730
adrn merged 4 commits into
astropy:mainfrom
mhvk:coordinate-skyoffset-origin-bugs

mhvk commented May 11, 2021

Uh oh!

mhvk commented May 11, 2021

Uh oh!

mhvk May 11, 2021

Uh oh!

nstarman May 16, 2021

Uh oh!

nstarman May 16, 2021 •

edited

Loading

Uh oh!

mhvk May 16, 2021

Uh oh!

mhvk commented May 16, 2021

Uh oh!

nstarman commented May 16, 2021 •

edited

Loading

Uh oh!

adrn left a comment •

edited

Loading

Uh oh!

adrn commented May 24, 2021

Uh oh!

mhvk commented May 24, 2021

Uh oh!

adrn commented May 24, 2021

Uh oh!

eteq commented Jun 11, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		from astropy.tests.helper import assert_quantity_allclose as assert_allclose


		def test_altaz_attribute_transforms():

	frame_kwargs.pop('origin', None)
	if self.frame.__class__ is SkyOffsetFrame:
	frame_kwargs.pop('origin', None)

Conversation

mhvk commented May 11, 2021

Uh oh!

mhvk commented May 11, 2021

Uh oh!

mhvk May 11, 2021

Choose a reason for hiding this comment

Uh oh!

nstarman May 16, 2021

Choose a reason for hiding this comment

Uh oh!

nstarman May 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk May 16, 2021

Choose a reason for hiding this comment

Uh oh!

mhvk commented May 16, 2021

Uh oh!

nstarman commented May 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adrn left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrn commented May 24, 2021

Uh oh!

mhvk commented May 24, 2021

Uh oh!

adrn commented May 24, 2021

Uh oh!

eteq commented Jun 11, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nstarman May 16, 2021 •

edited

Loading

nstarman commented May 16, 2021 •

edited

Loading

adrn left a comment •

edited

Loading