-
-
Notifications
You must be signed in to change notification settings - Fork 124
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
Compatibility with unreleased ASDF stack for JWST s2d data #1079
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1079 +/- ##
=======================================
Coverage 70.72% 70.73%
=======================================
Files 64 64
Lines 4485 4483 -2
=======================================
- Hits 3172 3171 -1
+ Misses 1313 1312 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Looks good to me except I'm not sure the astropy dev testing change is working. |
Good catch... |
But astropy 5.3.3 is also pulled into the devdeps job in unrelated PR (https://github.com/astropy/specutils/actions/runs/6147346407/job/16678644002). So something is pinning it but not caused by the changes here. |
It also doesn't appear to be grabbing numpy 2.0: :-| |
This comment was marked as resolved.
This comment was marked as resolved.
BTW numpy is fine, it is grabbing RC instead of dev but that is a known issue. |
14e7f32
to
c211be5
Compare
For my own sanity, I moved the other fixes out to separate PRs. Now this PR only deals with the parser. |
Wut?
|
@braingram , what is this? Is this really necessary in specutils? Lines 9 to 14 in 85d07a9
|
I don't think that's required for any of the asdf schema testing. The testing should be enabled by: Lines 63 to 64 in 85d07a9
See: https://asdf.readthedocs.io/en/latest/asdf/extending/schemas.html#testing-custom-schemas Testing locally by removing the lines I still see the schemas tests. The CI does currently test the schemas, see: |
I might have missed this on my first look but: specutils/specutils/io/default_loaders/jwst_reader.py Lines 570 to 573 in 85d07a9
needs a similar fix as the one applied in this PR |
because it should not operate on closed ASDF file handler.
c211be5
to
b6b0f44
Compare
Good catch, @braingram . I pushed new changes. Does it look good now? |
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.
Looks great! Thanks for fixing this.
I don't understand the GWCS error on devdeps job though. It seems unrelated but it is also blocking the test against asdf dev here. |
My suspicion is it has something to do with |
Oh, right... That would theoretically be fixed by #1081... So I guess I will merge this and worry about the devdeps in that PR. 🤪 Thanks! |
asdf 3.0 is released, so we also need to release this... 🐱 |
I'll release today, thanks for the ping. |
Thank YOU! |
This will hopefully address spacetelescope/jdaviz#2446 . Minimally reproducible example that crashes without this patch using unreleased ASDF stack:
🐱