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

Don't allow adding new frames to transform graph if their attributes conflict with component names #6871

Merged
merged 8 commits into from Dec 23, 2017

Conversation

adrn
Copy link
Member

@adrn adrn commented Nov 17, 2017

See #6870 for a description of the problem.

This solves the problem by checking whether a new frame class added to the transform graph has invalid attribute names.

@adrn adrn requested review from eteq and mhvk November 17, 2017 20:37
@astropy-bot
Copy link

astropy-bot bot commented Nov 17, 2017

Hi there @adrn 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

@adrn adrn added this to the v3.0.0 milestone Dec 8, 2017
@adrn
Copy link
Member Author

adrn commented Dec 9, 2017

Tests passed on last commit, so I just pushed up a changelog entry.

@adrn adrn force-pushed the coordinates/block-component-overrides branch from 1424a0f to 8f10da1 Compare December 9, 2017 16:53
Copy link
Member

@eteq eteq left a comment

Choose a reason for hiding this comment

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

Minor change needed of moving the changelog, plus possible change needed for my bigger concern below about backwards compatibility.

First, I did a quick check and this has no noticeable impact in the time needed to import coordinates, so that's good!

My concern, though, is that this is a backwards-incompatible change. Usually we give at least one version of deprecation warning. So maybe this error should be changed to a warning for this version, and then update to an error for 3.1?

CHANGES.rst Outdated
the ``.parallax`` attributes. [#6855]

- Coordinate frame classes now can't be added to the frame transform graph if
Copy link
Member

Choose a reason for hiding this comment

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

Technically this is an API change, right? Should probably put it there instead of "new feature"

@@ -38,6 +38,41 @@
'FunctionTransformWithFiniteDifference', 'CompositeTransform']


def frame_attrs_from_set(frame_set):
Copy link
Member

Choose a reason for hiding this comment

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

Do you mean for this (and the next function) to be part of the public API? I'm guessing not since it's not fully documented (and I'm OK with that), but if you do it needs to be in __all__.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I think they should be private.

@adrn
Copy link
Member Author

adrn commented Dec 19, 2017

Fair point RE: deprecation. I changed the error to a deprecation warning.

@adrn adrn force-pushed the coordinates/block-component-overrides branch from 8f10da1 to 6d996e8 Compare December 19, 2017 01:03
@adrn
Copy link
Member Author

adrn commented Dec 19, 2017

Actually, even though we allow any attribute names right now, it breaks SkyCoord. For example, in v2.0.2:

import astropy.coordinates as coord
import astropy.units as u

class BorkedFrame(coord.BaseCoordinateFrame):
    ra = coord.Attribute(default=150)
    dec = coord.Attribute(default=150)

def trans_func(coo1, f):
    pass

trans = coord.FunctionTransform(trans_func, BorkedFrame, coord.ICRS)
trans.register(coord.frame_transform_graph)

icrs = coord.SkyCoord(ra=160*u.deg, dec=-11*u.deg)
icrs.transform_to(coord.FK5)

results in a

ValueError: The frame object "<FK5 Frame (equinox=J2000.000)>" does not have associated data

So IMO it's somewhere in between an API change and a bugfix. Thoughts on whether this should raise a warning or error?

CHANGES.rst Outdated
@@ -25,6 +25,9 @@ astropy.coordinates
properties that provide shorthands to the full-space Cartesian velocity as
a ``CartesianDifferential``, the 2D proper motion as a ``Quantity``, and the
radial or line-of-sight velocity as a ``Quantity``. [#6869]
- Coordinate frame classes now can't be added to the frame transform graph if
Copy link
Member

Choose a reason for hiding this comment

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

need a blank line above (rebase issue?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops - yes...

@eteq
Copy link
Member

eteq commented Dec 22, 2017

re: @adrn's

Thoughts on whether this should raise a warning or error?

I think it's still technically an API change, because the warning/error now happens earlier. But you're right it's less likely anyone will have used this because it will make SkyCoord transformations useless for them. By policy we should still have one version of warning and then switch to exception in 3.1 ... but maybe this is dangerous enough that we should violate the policy on the theory that this is really going to make trouble for anyone working on this anyway?

Also, @adrn, I think I know why the tests are failing in a somewhat unpredictible way: the test that adds the BorkedFrame currently leave the frame transform graph in a borked state, so if later tests use it, they run into the problem you pointed out above. I think you need to explicitly . Ironically, this would be fixed if you use an error instead of a warning above, because then it'll never actually get hooked up in the graph... So maybe that's an argument for going straight to the error now?

@adrn
Copy link
Member Author

adrn commented Dec 22, 2017

I think I would prefer to error now: it's more work to deprecate and deal with cleaning up the frame_transform_graph for a hypothetical (super)user who (a) defined their own frame and added it to the transform graph, and (b) already has broken astropy.coordinates (or at least SkyCoord)! But I'll leave it to a member of the coordination committee to decide 😉

@adrn adrn force-pushed the coordinates/block-component-overrides branch from 6d996e8 to 6f027a0 Compare December 22, 2017 22:08
@eteq eteq merged commit e556eca into astropy:master Dec 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants