SkyCoord/Frame classes don't respect wrap angle when velocity data exists

@Cadair

I have some frames that use a @Cadair trick to set the default wrap angle of the longitude component to be 180º, e.g.:

class ICRS180(coord.ICRS):
    def __init__(self, *args, **kwargs):
        wrap = kwargs.pop('wrap_longitude', True)
        super().__init__(*args, **kwargs)
        if wrap and isinstance(self._data, (coord.UnitSphericalRepresentation,
                                            coord.SphericalRepresentation)):
            self._data.lon.wrap_angle = 180*u.deg
            
@coord.frame_transform_graph.transform(coord.StaticMatrixTransform, coord.ICRS, ICRS180)
def icrs_to_180():
    return np.eye(3)

@coord.frame_transform_graph.transform(coord.StaticMatrixTransform, ICRS180, coord.ICRS)
def icrs180_to_icrs():
    return np.eye(3)

If I transform to this frame using positional data only, everything looks cool, i.e. the longitude gets wrapped as expected:

>>> coord.SkyCoord(ra=270*u.deg, dec=-11*u.deg).transform_to(ICRS180)
<SkyCoord (ICRS180): (ra, dec) in deg
    (-90., -11.)>

But when I include proper motion information, the wrap is no longer respected:

>>> coord.SkyCoord(ra=270*u.deg, dec=-11*u.deg,
...                pm_ra_cosdec=11*u.mas/u.yr, pm_dec=0*u.mas/u.yr).transform_to(ICRS180)
<SkyCoord (ICRS180): (ra, dec) in deg
    (270., -11.)
 (pm_ra_cosdec, pm_dec) in mas / yr
    (11., 4.750875e-32)>

This happens when using the frame classes directly, or SkyCoord, so I think it's a bug in the frame classes. I'll try to take a look at this soon.

cc @eteq @mhvk

I dug a bit further into why this happens. It looks like it's because of a round-trip to Cartesian that happens when represent_as() is called in the frame classes inside __getattr__() (here, I think). In transforming to then from cartesian, it loses the wrap_angle.

One way to solve it is to allow representation (and differential?) objects to have special attributes that get carried along. Any other ideas?

@eteq

Here's another way to phrase the underlying issue in the context of a frame that already exists. With the Galactic coordinate system, different people have different conventions about whether the longitude l wraps at 180º or 360º. Right now, we don't really support both (we just have our default, to wrap at 360º). Example:

>>> c = coord.Galactic(l=-50*u.deg, b=0*u.deg)
>>> c
<Galactic Coordinate: (l, b) in deg
    ( 310.,  0.)>
>>> c.l.wrap_angle = 180*u.deg
>>> c.l
−50∘00′00′′
>>> c
<Galactic Coordinate: (l, b) in deg
    ( 310.,  0.)>

The frame doesn't preserve the wrap angle because it round-trips to a cartesian representation (I think).

@eteq and I were just discussing this on the phone and came up with two ideas for how to proceed.

Option 1:

Proposal: Add a new type of attribute to frame classes for handling representation/differential information and make sure this propagates when the class is replicated (see _replicate()). Conceptually, we wouldn't add this to the frame attributes because those currently only relate to the specification of the reference frame, not the representation of the data.
Advantage: The attribute would be controllable at the instance level, so we could use the same Galactic frame class but let the user control, e.g., the longitude wrap angle
Disadvantage: Is this over-engineering? This also creates a new set of attributes to carry along and check / support.

Option 2:

Proposal: Add a "wrap_angle" or "wrapangle" keyword argument to RepresentationMapping.
Advantage: Simpler implementation?
Disadvantage: Wrapping is defined at the class-level, meaning we would need to add a Galactic180 class with a different wrapping convention in order to support what is described above.

Any thoughts @mhvk ?

@adrn

@adrn - I think the example you gave on top and the one just above reflect two different issues in the implementation - what happens in transformations and how the frame is represented and can be adjusted (the latter a long-standing annoyance of mine - #4903, #6184). So, when you see coordinates in degrees, that is not because the actual underlying data is in degrees:

c = coord.Galactic(l=-50*u.deg, b=0*u.deg)
# Input in degrees, hence data in degrees
c.data
# <UnitSphericalRepresentation (lon, lat) in deg
#     (-50., 0.)>
c.transform_to(coord.ICRS).data
# Transformation via Cartesian leads to radians
# <UnitSphericalRepresentation (lon, lat) in rad
#    (3.6315327, -1.08269481)>

But what you get to see in repr is always in degrees - it is a copy of the real data.

Of course, in reality how data is presented to a user is not all that important - any arithmetic doesn't depend on the unit or the wrap angle.

But since we're talking about presentation, it makes most sense to me to have the presentation of any frame be user-controlled (i.e., if I want hms, that should be possible), and I would count wrap_angle among that. It might make most sense as some general settings, say coordinates.print_options or so...

More concretely: in my ideal world (i.e., #4903), frames have __format__ (or to_string) method that allows one to determine how they are formatted, and the default is setting via some configuration item. For longitudes, the format would not just allow deg vs hms, etc., but also a wrapping angle. Possibly, this could start with defining __format__ for Angle, Latitude and Longitude. Though I recall that an early attempt to define some mini-language quickly became a bike-shedding exercise, so maybe it is easier to start with to_string, which already exists for Angle and could use a wrap_angle update for Longitude.

To resurrect this discussion, I just wanted to add one new thought. I don't think this is merely a presentation / display issue. For some of my custom frames in Gala, I want to be able to use the longitude attribute with the assumption that it is always wrapped to 180 deg, for example in plotting, or in selecting ranges in longitude, e.g., (where lon = phi1 and lat = phi2)

plt.plot(c.phi1, c.phi2)

but instead have to do:

plt.plot(c.phi1.wrap_at(180*u.deg), c.phi2)

or for selecting ranges:

mask = (c.phi1 > -20*u.deg) & (c.phi1 < 40*u.deg)

but instead have to do:

mask = (c.phi1.wrap_at(180*u.deg) > -20*u.deg) & (c.phi1.wrap_at(180*u.deg) < 40*u.deg)

@adrn

@adrn - Hmm, above you are using the data that are cached for the representation, not the actual underlying numbers, and perhaps that gives us in fact some leeway, by ensuring the cached representations are always wrapped. Indeed, the following seems to work:

class ICRS180(coord.ICRS):
    def represent_as(self, base, s='base', in_frame_units=False):
        data = super().represent_as(base, s, in_frame_units=in_frame_units)
        if isinstance(data, (coord.SphericalRepresentation, coord.UnitSphericalRepresentation)):
            data.lon.wrap_angle=180.*u.deg
        return data

@coord.frame_transform_graph.transform(coord.StaticMatrixTransform, coord.ICRS, ICRS180)
def icrs_to_180():
    return np.eye(3)

@coord.frame_transform_graph.transform(coord.StaticMatrixTransform, ICRS180, coord.ICRS)
def icrs180_to_icrs():
    return np.eye(3)

coord.SkyCoord(ra=270*u.deg, dec=-11*u.deg).transform_to(ICRS180)
# <SkyCoord (ICRS180): (ra, dec) in deg
#     (-90., -11.)>
coord.SkyCoord(ra=270*u.deg, dec=-11*u.deg, pm_ra_cosdec=11*u.mas/u.yr, pm_dec=0*u.mas/u.yr).transform_to(ICRS180)
# <SkyCoord (ICRS180): (ra, dec) in deg
#     (-90., -11.)
#  (pm_ra_cosdec, pm_dec) in mas / yr
#     (11., 4.93038066e-32)>

This does suggest we could do it with some kind of frame attribute, which is used in represent_as and which, when set, simply clears the cache.

I do like that this way we avoid having to define differently wrapped representation classes...

@mhvk

Hi,

@mhvk

Do we have some update on this? I just found that the SkyoffsetFrame class also suffers the issue that if the data are requested through represent_as() , the wrap_angle may not be 180.0 degree. This happens when the underlying data is in representation_type of e.g. cartesian.

It seems that overwriting represent_as in the SkyoffsetFrame class is a viable approach, as demonstrated in the above referred PR for sunpy.

Any thought?

I'll be happy to put a PR for this if it makes sense.

Yes, it looks like sunpy followed my suggestion above. Of course, for their case it is a bit easier since mostly they want to change the default, while for SkyOffsetFrame, one would want to introduce the option to have a different wrap angle. But I could imagine a keyword argument wrap_longitude which would be False (no wrapping set), True (default wrapping angle), or a specific user-specified angle. I guess the best place for such an argument would be in represent_as() - at least, I don't much like sticking presentation information into the initializer.

I am not sure one would want a different wrapping angle for SkyOffsetFrame. As far as I understand, the SkyOffsetFrame works as a local 2d cartesian-like coordinate, therefore the only wrapping angle that would make sense is 180 degree.

The current implementation set the wrapping angle to 180 degree in the __init__, which does not work for cases where data are stored in cartesian representation.

@Jerry-Ma

@Jerry-Ma - you're right, since SkyOffsetFrame already has a default of 180 degrees for the wrap angle, there is nothing against ensuring that default also is used when converting from Cartesian. So, yes, if you can make a PR to do that, that would be great! Of course, it also means it cannot quite solve the actual issue here, since Galactic does not have such a default. But easiest to keep the issue separate.

Navigation Menu

Repository navigation

SkyCoord/Frame classes don't respect wrap angle when velocity data exists #7462

Option 1:

Option 2:

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

SkyCoord/Frame classes don't respect wrap angle when velocity data exists #7462

Description

Activity

adrn commented on Sep 18, 2018

adrn commented on Sep 18, 2018

Option 1:

Option 2:

mhvk commented on Sep 18, 2018

mhvk commented on Sep 18, 2018

adrn commented on Apr 12, 2019

mhvk commented on Apr 12, 2019

Jerry-Ma commented on Apr 17, 2020

mhvk commented on Apr 17, 2020

Jerry-Ma commented on Apr 17, 2020

mhvk commented on Apr 17, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions