Ensure FrameAttribute not on present frame are set on SkyCoord, not on its frame #5750

mhvk · 2017-01-29T17:12:14Z

Currently, if one defines, e.g., a SkyCoord with an ICRS frame, one can set any existing frame attribute that is not on that frame. Oddly, this gets set on the frame, without any validation:

sc = SkyCoord(ra=0., dec=0., unit='deg')
sc.relative_humidity = 'bla'
sc.frame.relative_humidity
# 'bla'

This commit ~~prevents one from setting any attribute that is a FrameAttribute of any frame~~ [EDIT] ensures that possible frame attributes that are not on the present frame are stored on the SkyCoord instance.

EDIT: now went with alternative given that the setting is used in the wild (for sensible purposes).

@taldcroft, @eteq - please check this is in fact desired behaviour; the alternative would be to set the attribute on the SkyCoord itself, but my sense was that this violated the immutability.

mhvk · 2017-01-29T19:05:48Z

p.s. I found this while working on #5751 -- if we do allow setting the attributes, I would suggest it to be once-only, i.e., once one sets, e.g., obstime on an ICRS SkyCoord, be it via the initializer or via assignment, sc.obstime becomes immutable.

taldcroft · 2017-01-29T22:50:07Z

@mhvk - I'm worried that the ability to update non-frame attributes has been around since SkyCoord came into existence and it may be out there in use. This would break code like:

sc = yaml.load('icrs_skycoord.yaml')  # No control over initializer
sc.obstime = '2015:001'
sc_fk4 = sc_icrs.transform_to('fk4')

Yes, you can do this with transform_to(FK4(obstime='2015:001')), but probably if I were writing code without thinking too hard I would use the above. I like encapsulating the attribute in my object rather than making it a function of the transform. I'm not sure if SkyCoord is formally considered immutable so much as the data frame is immutable.

mhvk · 2017-01-30T01:26:02Z

@taldcroft - yes, I worried about that too, hence my postscript about maybe relaxing this to allow an attribute to be set if it hasn't been set already. Shall I change it to work like that?

taldcroft · 2017-01-30T11:42:29Z

@mhvk - OK, the immutable-once-set idea sounds fine.

Cadair · 2017-01-30T11:45:42Z

I am heavily 👎 on this, either if it has been set or not, as changing the attributes of the solar frames is critical in allowing us to convert between systems with different observers. A full example of this is here: http://docs.sunpy.org/en/stable/generated/gallery/tutorials/SDO_to_STEREO_Coordinate_Conversion.html .

Basically what happens is an image might have an observer for a satellite and another might have an observer for the Earth, if we change out of an observer centric frame then change the observer location frame attributes on the frame where they are not being used, and then transform back we end up in a frame from a different place, which is critical for comparing locations from different view points.

Cadair · 2017-01-30T11:46:58Z

I notice that this has been milestoned as 1.3.1, this change should not be in a bug fix release!!

Cadair · 2017-01-30T11:52:34Z

We could modify our internal transformation code (https://github.com/sunpy/sunpy/blob/master/sunpy/coordinates/transformations.py#L192) to use @taldcroft's (transform_to(HelioprojectiveFrame(L0=..., B0=,...)) example. However, we have advertised this as functionality of sunpy.coordinates so I think it needs to be thought about a bit more. (I am not really sure what you are trying to prevent with this patch).

mhvk · 2017-01-30T14:55:21Z

@Cadair - To see why something like this is necessary, see #5751. But this really is two things: a frame should not have attributes that are not part of its regular definition (while a SkyCoord can), and, should a SkyCoord be immutable.

I think from the above, the consensus certainly is that one should be able to initialise other-frame attributes on an existing SkyCoord instance. From what you say, it sounds like you have existing code that relies on also changing such attributes, correct? If so, I think the correct thing to do is to still allow that. @taldcroft - what do you think?

If we go that route, then this PR would only ensure that attributes get stored in the correct location, and #5751 would add that, whenever they get set, they also get validated.

mhvk · 2017-01-30T14:59:28Z

p.s. @taldcroft, @Cadair - if you both think it is OK, it is perhaps easier to just do this in the context of #5751 (I don't really know any more why I had the two PRs...).

Cadair · 2017-01-30T15:24:58Z

Yes, we use the fact you can change existing attributes on a SkyCoord to swap observers. (Which is a transformation from one frame into another and out to the same frame with a different attribute).

I think limiting the setting of SkyCoord attributes to only ones that exist on frames is ok, as long as it's not a massive performance hit. I think we should keep the ability for the attributes to be modified if they are not the active frame though, as that's what we are using (exploiting?!) to do this observer switch-eroo.

mhvk · 2017-02-01T21:56:20Z

@taldcroft, @Cadair - I now reversed the logic, and made this PR set any frame attribute that is not on its present frame on the SkyCoord instance (instead of its frame).

Combining this with #5751 will ensure it will also be validated.

Does this make sense?

mhvk · 2017-02-07T19:48:34Z

@Cadair - I think the present PR would not break anything, could you have a check?
@taldcroft - any objections to this?

eteq · 2017-02-16T22:48:50Z

@mhvk - I like the present solution in this PR, if I'm understanding correctly... I actually thought this was the current behavior until seeing this PR and the associated issues. So in that sense I'm fine with calling this a bug fix (especially given #5751) But I would like to hear from @Cadair if this is compatible with the Sunpy use case before merging.

Cadair · 2017-02-16T23:06:38Z

I will try and test this tomorrow.

taldcroft

Finished my review. In its current state this doesn't seem controversial. I honestly don't know why I put "non-frame" attributes into the frame instance instead of the SkyCoord instance, but I agree this wasn't the best choice.

taldcroft · 2017-02-17T00:48:15Z

astropy/coordinates/sky_coordinate.py

-                (not attr.startswith('_') and
-                 hasattr(self._sky_coord_frame, attr))):
+            if not attr.startswith('_') and hasattr(self._sky_coord_frame,
+                                                    attr):


I personally think this is a nice example of where a strict 80 character line length limit makes the code less readable, not more. The existing style in this module is up to 100 characters, so in the future please feel free to let lines go somewhat past 80 if that avoids an awkward line break.

Alternatively the previous version had a bit more logical line break

taldcroft · 2017-02-17T01:49:58Z

CHANGES.rst

@@ -185,6 +185,12 @@ Bug Fixes
  - Fixed a bug where ``get_transform`` could sometimes produce confusing errors
    because of a typo in the input validation. [#5645]

+  - Ensured that frame attributes cannot be set on ``SkyCoord`` if the current


This is pretty confusing, and I think the first part was already true. What about:

Changed SkyCoord so that frame attributes which are not valid for the current frame (but are valid for other frames) are stored on the SkyCoord instance instead of the underlying frame instance. An example is setting relative_humidity on an ICRS SkyCoord instance.

Currently, if one defines, e.g., a SkyCoord with an ICRS frame, one can set any existing frame attribute that is not on that frame. Oddly, this gets set on the frame, without any validation: ``` sc = SkyCoord(ra=0., dec=0., unit='deg') sc.relative_humidity = 'bla' sc.frame.relative_humidity ``` This commit prevents one from setting any attribute that is a FrameAttribute of any frame.

Discussions showed that this is actually useful, so unlike in the previous commit, we should not forbid such setting. However, it still makes no sense to store those non-frame attributes on the frame; instead, they are now stored on the SkyCoord instance. (Validation of these is deferred to further commits.)

mhvk · 2017-02-17T02:18:49Z

@taldcroft - OK, done both (much better text indeed). Rebased for good measure.

Cadair

This doesn't seem to break anything in release SunPy. I have another problem that I discovered while testing this but it seems to be unrelated to this.

mhvk · 2017-02-17T14:38:22Z

OK, I'll merge so that I can get my follow-up PR rebased on master, and it will be clearer what is new.

Ensure FrameAttribute not on present frame are set on SkyCoord, not on its frame

mhvk added Bug coordinates labels Jan 29, 2017

mhvk added this to the v1.3.1 milestone Jan 29, 2017

mhvk mentioned this pull request Jan 29, 2017

Skycoord attributes slicing, dicing, propagation #5751

Merged

mhvk force-pushed the skycoord-setattr-bug branch from 0e5d66d to e302575 Compare February 1, 2017 21:51

mhvk changed the title ~~Ensure no existing FrameAttribute can be set on a SkyCoord.~~ Ensure FrameAttribute not on present frame are set on SkyCoord, not on its frame Feb 1, 2017

mhvk force-pushed the skycoord-setattr-bug branch from e302575 to b47fbce Compare February 1, 2017 22:03

mhvk added the Ready-for-final-review label Feb 7, 2017

taldcroft requested changes Feb 17, 2017

View reviewed changes

mhvk added 3 commits February 16, 2017 21:15

small fixups following Tom Aldcroft's comments

dcdb167

mhvk force-pushed the skycoord-setattr-bug branch from b47fbce to dcdb167 Compare February 17, 2017 02:18

taldcroft approved these changes Feb 17, 2017

View reviewed changes

Cadair approved these changes Feb 17, 2017

View reviewed changes

mhvk merged commit 9d64f44 into astropy:master Feb 17, 2017

mhvk deleted the skycoord-setattr-bug branch February 17, 2017 14:38

bsipocz pushed a commit that referenced this pull request Feb 22, 2017

Merge pull request #5750 from mhvk/skycoord-setattr-bug

7d0728d

Ensure FrameAttribute not on present frame are set on SkyCoord, not on its frame

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure FrameAttribute not on present frame are set on SkyCoord, not on its frame #5750

Ensure FrameAttribute not on present frame are set on SkyCoord, not on its frame #5750

mhvk commented Jan 29, 2017 •

edited

mhvk commented Jan 29, 2017

taldcroft commented Jan 29, 2017

mhvk commented Jan 30, 2017

taldcroft commented Jan 30, 2017

Cadair commented Jan 30, 2017 •

edited

Cadair commented Jan 30, 2017

Cadair commented Jan 30, 2017 •

edited

mhvk commented Jan 30, 2017

mhvk commented Jan 30, 2017

Cadair commented Jan 30, 2017

mhvk commented Feb 1, 2017

mhvk commented Feb 7, 2017

eteq commented Feb 16, 2017

Cadair commented Feb 16, 2017

taldcroft left a comment

taldcroft Feb 17, 2017

bsipocz Feb 17, 2017

taldcroft Feb 17, 2017

mhvk commented Feb 17, 2017

Cadair left a comment

mhvk commented Feb 17, 2017

Ensure FrameAttribute not on present frame are set on SkyCoord, not on its frame #5750

Ensure FrameAttribute not on present frame are set on SkyCoord, not on its frame #5750

Conversation

mhvk commented Jan 29, 2017 • edited

mhvk commented Jan 29, 2017

taldcroft commented Jan 29, 2017

mhvk commented Jan 30, 2017

taldcroft commented Jan 30, 2017

Cadair commented Jan 30, 2017 • edited

Cadair commented Jan 30, 2017

Cadair commented Jan 30, 2017 • edited

mhvk commented Jan 30, 2017

mhvk commented Jan 30, 2017

Cadair commented Jan 30, 2017

mhvk commented Feb 1, 2017

mhvk commented Feb 7, 2017

eteq commented Feb 16, 2017

Cadair commented Feb 16, 2017

taldcroft left a comment

Choose a reason for hiding this comment

taldcroft Feb 17, 2017

Choose a reason for hiding this comment

bsipocz Feb 17, 2017

Choose a reason for hiding this comment

taldcroft Feb 17, 2017

Choose a reason for hiding this comment

mhvk commented Feb 17, 2017

Cadair left a comment

Choose a reason for hiding this comment

mhvk commented Feb 17, 2017

mhvk commented Jan 29, 2017 •

edited

Cadair commented Jan 30, 2017 •

edited

Cadair commented Jan 30, 2017 •

edited