-
Notifications
You must be signed in to change notification settings - Fork 57
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
Upgrade jsonschema to 3.0.1 #684
Upgrade jsonschema to 3.0.1 #684
Conversation
Hi there @eslavich 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog. I help make 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 😃. I noticed the following issue with this pull request:
Would it be possible to fix this? Thanks! If there are any issues with this message, please report them here. |
Codecov Report
@@ Coverage Diff @@
## master #684 +/- ##
==========================================
- Coverage 93.45% 93.39% -0.06%
==========================================
Files 39 39
Lines 4341 4347 +6
==========================================
+ Hits 4057 4060 +3
- Misses 284 287 +3
Continue to review full report at Codecov.
|
This looks good! Is it possible to have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Needs a change log. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good.
Both jsonschema 3.0.1
and jsonschema 2.3.0
are being tested in the TravisCI matrix. The 2.3.0
build is here
https://travis-ci.org/spacetelescope/asdf/jobs/549785630
I think that is not reflected in codecov. That's fine by me.
I don't understand why this did not solve my problem trying to load this JWST image: https://stsci.box.com/s/hwrc5reqygmmv2rl3yvvz90l7ryjac6h
from astropy.io import fits
from asdf.fits_embed import AsdfInFits
filename = 'jw1069001001_01203_00002_nrca1_level2_cal.fits'
pf = fits.open(filename)
asdf_f = AsdfInFits.open(pf) I get this:
What is going on? |
The released version of |
@jdavies-st , I am using dev version of |
Excellent and you're still seeing this issue in the dev version? |
Yes, it appears to pass the install check but something crashes when I read the file in the Box link above. You can try to reproduce it yourself, and if it works for you, then at least I know it is just me. |
I suspect that because |
In that case... Is there a plan for |
Yes, there is a plan and PR ^ But I'm not sure we want to do this before a new release of |
I ran into this jsonschema version issue also. I think most (all?) asdf users will run into this issue at the moment? Using asdf master resolved it, but it requires quite a dance and the commands aren't given here
Is it possible to release a new stable asdf version with this fix soon? |
@cdeil Yes, we will have a release out early next week. Your gist example initializes a Gaussian model and this reminds me the Gaussian doesn't have a schema yet. We need to add schemas for all models but it'd be good to know what models you need to serialize and add those schemas first. Even better if someone else can add them :). |
Great!
In Gammapy, we currently have our own models, not using Astropy models yet. E.g.:
And now we are adding config files and serialisation, my comment was triggered by gammapy/gammapy#2314 (comment) I don't know if we can / will switch to use astropy.modeling and ASDF, it's an option we'll investigate. So for now, from our side, there's no need to add or prioritise any Astropy models. cc @QRemy and @adonath who will work on model serialisation in Gammapy. If you have time to make a PR for Gaussian model for Astropy, that might be a good opportunity to learn how it works and to make a contribution. @nden - Adding a model means writing code like this? |
@cdeil Yes, the tag is implemented in a class defining the two methods |
Where is the schema e.g. for ConstantType? I see there's schemas in astropy/io/misc/asdf/data/schemas/astropy.org/astropy, but I couldn't find any for models?
@nden - Great! |
They are in spacetelescope/asdf-standard. Yes, we are discussing whether this kind of separation makes sense. It does if there were other language implementations, but until then, it's more awkward to manage. |
Ah, it's here: https://github.com/spacetelescope/asdf-standard/blob/master/schemas/stsci.edu/asdf/transform/constant-1.3.0.yaml I see, yes, that's a difficult decision then where to maintain the schemas. |
Addresses #530
In its current state, this PR isn't compatible with jsonschema < 3, due to some (relatively minor) changes to
validators.create(...)
. It would be straightforward to support both, but I thought I'd keep it clean unless I hear otherwise.I installed this and ran the unit tests on jwst, gwcs, and astropy. With the changes in spacetelescope/jwst#3705, they all pass.