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

Make asdf schemas and tags support more coordinates and representations #7079

Merged
merged 5 commits into from Mar 2, 2018

Conversation

@Cadair
Copy link
Member

Cadair commented Jan 12, 2018

This is the first bit of work needed to get asdf to save the SunPy coordinate frames. In the process I hope to generalise the asdf tags and schemas for frames so that everything that's BaseCoordinateFrame can be saved.

So far this contains:

  • Tags and Schemas for:
    • Angle
    • Latitude
    • Longitude
    • All representation classes
    • ICRS and FK5 frames

After a few rounds of iteration the way this is implemented for representations is there is one schema and one tag, the tag can save any representation which is a subclass of BaseRepresentationorDifferential, the schema for this lists all the currently implemented representations and differentials and the components which they accept.

For frames, there is one tag class which can save anything that's a subclass of BaseCoordinateFrame, however, there has to be an explicit schema for that frame. This is to ensure that the frame in an asdf file can be validated, mainly to be explicit about the attributes defined on the frames. Also it makes it easier for libs like SunPy to implement schemas (and a subclass of the tag) for frames not defined in Astropy.

@astropy-bot

This comment has been minimized.

Copy link

astropy-bot bot commented Jan 12, 2018

Hi there @Cadair 👋 - 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.

@drdavella drdavella self-requested a review Jan 12, 2018

def test_unitspherical180(tmpdir):
x = UnitSphericalWrap180Representation(-10*u.deg, 10*u.deg)
# Without this here this test fails.

This comment has been minimized.

Copy link
@Cadair

Cadair Jan 12, 2018

Author Member

If anyone more familiar with the insane inner workings of representations (@eteq @adrn ?) has any ideas what might be causing this please let me know!

Copy link
Contributor

drdavella left a comment

Other than the weird error related to representation below, this LGTM. Thanks @Cadair!

requires = ['astropy']
version = "1.0.0"
organization = 'astropy.org'
standard = 'astropy'

This comment has been minimized.

Copy link
@drdavella

drdavella Jan 12, 2018

Contributor

Might be useful to have a comment here saying that we inherit from QuantityType, whose schema lives in stsci.edu:asdf, but we need to override that here since AngleType's schema is provided by astropy.org:astropy.

node['lat'] = custom_tree_to_tagged_tree(representation.lat, ctx)
# Force a convert to Longitude here, it seems that asdf has some issues
# with subclasses thereof in `custom_tree_to_tagged_tree`
node['lon'] = custom_tree_to_tagged_tree(Longitude(representation.lon), ctx)

This comment has been minimized.

Copy link
@Cadair

Cadair Jan 15, 2018

Author Member

@drdavella It seems that there is weridness with passing subclasses of classes that have schemas and tags. It's the same here and with the subclasses of representation in SunPy.

@drdavella

This comment has been minimized.

Copy link
Contributor

drdavella commented Jan 16, 2018

@Cadair I'm trying to run this locally but I keep getting an error related to spherical-1.0.0. Did you forget to commit this file?



REPRESENTATION_MODULE_WHITELIST = ['astropy.coordinates.representation',
'sunpy.coordinates.representation']

This comment has been minimized.

Copy link
@astrofrog

astrofrog Jan 16, 2018

Member

Just curious, should sunpy register its own entry points to avoid mentions to sunpy here?

This comment has been minimized.

Copy link
@Cadair

Cadair Jan 17, 2018

Author Member

It would be substantially more complex that way. This whitelist would basically have to become an entrypoint, which actually as I think about it makes it a slight security risk. If I could install pwn.me as a module and then read an asdf file with that module path in it would get executed. (Ignoring the fact that installing it in the first place is executing the code yada yada).

@Cadair Cadair force-pushed the Cadair:asdf_tags_coords branch from ddbde00 to 2e74d8b Jan 18, 2018
tag: "tag:astropy.org:astropy/coordinates/frame-1.0.0"

title: |
Represents an ICRS coordinate object from astropy

This comment has been minimized.

Copy link
@nden

nden Jan 18, 2018

Contributor

Is this a schema also fo rhe low level built in frames? If so the title and description need an update.

frame:
type: string
data:
anyOf:

This comment has been minimized.

Copy link
@nden

nden Jan 18, 2018

Contributor

This anyOf is not necessary.

This comment has been minimized.

Copy link
@nden

nden Jan 18, 2018

Contributor

Should enum be used here to list the possible names of frames supported in coordinates?

tag: "tag:astropy.org:astropy/coordinates/representation-1.0.0"

title: |
Representation of things

This comment has been minimized.

Copy link
@nden

nden Jan 18, 2018

Contributor

It's a bit general :-). Perhaps change things to coordinates.

type: object
properties:
type:
type: string

This comment has been minimized.

Copy link
@nden

nden Jan 18, 2018

Contributor

Enumerate the possible values?

This comment has been minimized.

Copy link
@Cadair

Cadair Jan 24, 2018

Author Member

done

@nden

This comment has been minimized.

Copy link
Contributor

nden commented Jan 18, 2018

@Cadair I guess the main question for me is do you think we should maintain separate schemas for frames and coordinates (as asdf did) ? I am a bit confused by the current naming.

@Cadair

This comment has been minimized.

Copy link
Member Author

Cadair commented Jan 18, 2018

The objective here was to write a general tag for BaseCoordinateFrame, so that with one tag would be able to save out any astropy or sunpy frame. Obviously to do this I needed to be able to serialise any representation and differential object, so I added a schema and tag for the base class for them. The frame attributes seem to all already have tags, so that was simple enough.

I don't particularly want to enumerate the potential values of the frame or representation names, as to my mind that defeats one of the objectives, any new frames or representations won't need changes to the schemas / tags.

Does that make any sense?

@nden

This comment has been minimized.

Copy link
Contributor

nden commented Jan 19, 2018

@Cadair Sure, it will serialize new frames but it won't validate them. It's so general that you can serialize almost anything. I'm not against these schemas but should we have more specific ones on top of this frame?
Also it sounds like sunpy plans to rely on these schemas/tags and not have its own asdf extension (just curious)?

@Cadair

This comment has been minimized.

Copy link
Member Author

Cadair commented Jan 23, 2018

@nden I have updated the frames part of this to use the tagged trickery from the transforms tag. Does this make sense to you now, with respect to schema / validation.

Obviously I need to do a similar thing for representation as well.

@Cadair Cadair force-pushed the Cadair:asdf_tags_coords branch 2 times, most recently from d6779b3 to 9cf4b52 Jan 24, 2018
Copy link
Contributor

nden left a comment

@Cadair I like where this is going.
One thing that I found extremely useful when writing schemas is adding example sections to all schemas. These get tested and often found problems which are not easy to spot otherwise. I think in general relying only on testing tags is not sufficient.

I also ran interactively an example with a galactocentric frame - something that gave us a lot of headaches before. It doesn't seem to work. Is this coming later?

I will try to run this with gwcs, hopefully this week and see how it goes.
Thanks for the work!


tree = {'coord': ICRS(ra=ra, dec=dec)}

assert_roundtrip_tree(tree, tmpdir, extensions=AstropyExtension())

This comment has been minimized.

Copy link
@nden

nden Jan 24, 2018

Contributor

Out of curiosity, is the extensions argument still needed? I thought it's registered as an entry point and we don't need to specify extensions any more.

This comment has been minimized.

Copy link
@drdavella

drdavella Feb 21, 2018

Contributor

It should not be necessary.

Represents a coordinate frame object from astropy
description:
This is the

This comment has been minimized.

Copy link
@nden

nden Jan 24, 2018

Contributor

This seems truncated.

@Cadair Cadair force-pushed the Cadair:asdf_tags_coords branch 3 times, most recently from 2a77a57 to 0a5eef8 Jan 24, 2018
@Cadair

This comment has been minimized.

Copy link
Member Author

Cadair commented Jan 24, 2018

@nden I have added examples to the schemas, I didn't add examples for all the representation classes because I thought that would make that schema file very long!

I also haven't written schemas for all the frames, I might try and do this, it will be time consuming and tedious (especially with the examples) but would be nice!

Cadair added a commit to Cadair/gwcs that referenced this pull request Jan 25, 2018
Cadair added a commit to Cadair/gwcs that referenced this pull request Jan 31, 2018
@drdavella

This comment has been minimized.

Copy link
Contributor

drdavella commented Feb 1, 2018

Current test failures are due to a bug in ASDF: spacetelescope/asdf#439. Once that is pulled in to asdf/master we should restart the test suite.


__all__ = ['ICRSCoordType']
data = node.get('data', None)
if data:

This comment has been minimized.

Copy link
@drdavella

drdavella Feb 1, 2018

Contributor

This needs to be if data is not None

components = {}
for c in comps:
value = getattr(representation, '_' + c, None)
if value:

This comment has been minimized.

Copy link
@drdavella

drdavella Feb 1, 2018

Contributor

This needs to be if value is not None

@Cadair Cadair force-pushed the Cadair:asdf_tags_coords branch from abfc456 to 34a2c68 Feb 2, 2018
@Cadair

This comment has been minimized.

Copy link
Member Author

Cadair commented Feb 12, 2018

argggghhh it's the same error on circleci this time :(

@pllim

This comment has been minimized.

Copy link
Member

pllim commented Feb 12, 2018

The rtol=1.5e-1 indicates that this is failing despite the fix at #7152. I am just mentioning it for the record so everyone doesn't need to download that log.

    def test_2d_model():
        ...
        m = fitter(p2, x, y, z + 2 * n, weights=None)
>       utils.assert_allclose(m.parameters, p2.parameters, rtol=1.5e-1)
E       AssertionError: 
E       Not equal to tolerance rtol=0.15, atol=0
E       
E       (mismatch 33.33333333333333%)
E        x: array([0.789448, 1.226486, 3.222042])
E        y: array([1. , 1.2, 3.2])
@Cadair Cadair force-pushed the Cadair:asdf_tags_coords branch from 80477dd to 102c09c Feb 12, 2018
@Cadair

This comment has been minimized.

Copy link
Member Author

Cadair commented Feb 12, 2018

@nden @drdavella I think this is good to go, assuming the unrelated CI bug goes away.

@Cadair Cadair force-pushed the Cadair:asdf_tags_coords branch from 102c09c to 52ae43d Feb 14, 2018
@drdavella drdavella self-requested a review Feb 21, 2018
@drdavella drdavella self-assigned this Feb 21, 2018
for fpath in files:
path, fname = os.path.split(fpath)
frame, _ = fname.split('-')
if frame not in ['baseframe', 'icrs']:

This comment has been minimized.

Copy link
@drdavella

drdavella Feb 21, 2018

Contributor

It would be useful to have a comment here about why these frames are ignored.

@drdavella

This comment has been minimized.

Copy link
Contributor

drdavella commented Feb 21, 2018

This looks good to me. @nden unless you have any objections I will merge this.

@nden
nden approved these changes Feb 22, 2018
CHANGES.rst Outdated
@@ -35,6 +35,8 @@ astropy.io.ascii
astropy.io.misc
^^^^^^^^^^^^^^^

Support for saving all representation classes and all frames that have schema has been added to the asdf submodule. [#7079]

This comment has been minimized.

Copy link
@nden

nden Feb 22, 2018

Contributor

It's probably just me but I think this can be a bit clearer. Perhaps something like "Added support for saving coordinate frames and representations to the asdf format."

@nden

This comment has been minimized.

Copy link
Contributor

nden commented Feb 22, 2018

@Cadair @drdavella I ran this PR with the corresponding PR in gwcs on jwst pipeline tests and all looks good. 👍 to merging and thanks for the work!

@drdavella

This comment has been minimized.

Copy link
Contributor

drdavella commented Feb 22, 2018

I'm good with merging, but what should we make of the test failures on circleci?

@nden

This comment has been minimized.

Copy link
Contributor

nden commented Feb 23, 2018

@Cadair Could you squash all commits and rebase. I think we'll see other unrelated failures after a rebase,
similar to the other open PRs.

@Cadair

This comment has been minimized.

Copy link
Member Author

Cadair commented Feb 23, 2018

Squash them all?! 😲

Will get to it tomorrow.

@Cadair Cadair force-pushed the Cadair:asdf_tags_coords branch from bea94a8 to f81777a Feb 26, 2018
@Cadair

This comment has been minimized.

Copy link
Member Author

Cadair commented Feb 26, 2018

The test failures appear to be #7214 related.

Cadair added a commit to Cadair/gwcs that referenced this pull request Feb 27, 2018
Cadair added 5 commits Jan 12, 2018
This is a combination of 8 commits.

- Add schemas for all frames but only one tag

  This allows validation of frame names and attributes allowed for the individual
  frames, while only having one tag for all frames everywhere.

- read the schema files for a list of frames

- Extend the representation schema to cover all astropy classes

  fixing up of frame schemas

- add a galactocentric schema for @nden

  fix baseframe id

- bump icrs to 1.1.0

- add examples to frame schemas

  Add examples in the schemas for angle, longitude and latitude

  Add a couple of representation examples

- Split the frame tag logic from the asdf bits.

  This makes it much much easier for SunPy to use

- Convert from strings to actual types
This set of frames is not complete, but they are all needed by gwcs.
@drdavella drdavella force-pushed the Cadair:asdf_tags_coords branch from f81777a to 161c840 Mar 2, 2018
@drdavella drdavella merged commit e9a286d into astropy:master Mar 2, 2018
8 checks passed
8 checks passed
astropy-bot All checks passed
Details
ci/circleci: 32bit Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl153 Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl202 Your tests passed on CircleCI!
Details
ci/circleci: image-tests-mpl212 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 84.751%
Details
@drdavella

This comment has been minimized.

Copy link
Contributor

drdavella commented Mar 2, 2018

Thanks @Cadair!

@Cadair

This comment has been minimized.

Copy link
Member Author

Cadair commented Mar 2, 2018

rejoice!!

@Cadair Cadair deleted the Cadair:asdf_tags_coords branch Mar 2, 2018
@drdavella drdavella mentioned this pull request Mar 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.