-
Notifications
You must be signed in to change notification settings - Fork 79
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Imviz parser #541
Imviz parser #541
Conversation
I don't have anything official to test with, so using one from my own stash: https://stsci.box.com/s/hwrc5reqygmmv2rl3yvvz90l7ryjac6h ( |
If you want a shortcut, the easiest would be to actually call |
Re: which images to treat as acceptance criteria - @pllim, I think you hit the two main items at the outset! I.e.
I think ideally we check both of those in CI following @astrofrog's suggestion in #541 (comment). That leaves open the question where to get the images. I think the answer for now is to try to pick a file for 1 that we're already using in some notebook (like maybe https://data.science.stsci.edu/redirect/JWST/jwst-data_analysis_tools/stellar_photometry/jw01072001001_01101_00001_nrcb1_cal.fits ? although I'm not 100% sure that's level 2, so might have to find another one) and for 2 get some convenient drz file on MAST - for example https://mast.stsci.edu/api/v0.1/Download/file?bundle_name=MAST_2021-04-21T1828.sh&uri=mast:HST/product/jclj01010_drz.fits would do just fine. |
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 good to me so far! Just a couple small comments below
4a60a4a
to
42f6bc3
Compare
@@ -2,15 +2,13 @@ | |||
"cells": [ | |||
{ | |||
"cell_type": "markdown", | |||
"id": "handed-person", |
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.
I have no idea how these got into the original notebook from Tom R. But when I update it, these are removed by jupyter
. 🤷
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.
Maybe a jupyter version difference? Fine to remove though :)
2091492
to
436ae00
Compare
The two test files given by Erik are not exactly small; hopefully not enough to trigger quota limits. 😬 🤞
|
After this is merged, if no further changes, @duytnguyendtn needs to update branch protection rules for CI, since I changed the job name a little. FYI. |
FWIW, the new tests passed locally... |
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 good to me! Thanks for adding the tests 😀
Great! @duytnguyendtn , any other approval needed for merge? |
This comment has been minimized.
This comment has been minimized.
Started writing tests. [ci skip]
e81d372
to
063f1ef
Compare
OK, bug fixed. Should be ready ready 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 good to me. I did some testing from the notebook (including loading the JWST file) as well as running the tests locally. I hadn't looked at the Imviz example notebook lately, Imviz is looking good so far!
Is there an issue open for the CI branch protection rule change you mentioned @pllim , or was that already addressed?
Wait, that last test needs to pass before I can merge this. But that test can't pass until @duytnguyendtn updates the branch protection rules for CI before the test will run? Is that right? |
No, it is now called "Python 3.8 with coverage checking, all deps, and remote data" but only after this PR is merged. We're stuck with chicken and egg situation. |
Updated the test name. Thanks for your patience! |
Fix #510
I cannot test this until #540 is resolved becausejwst
is not compatible with Windows.Reference: https://github.com/glue-viz/glue/blob/master/glue/core/data_factories/fits.py
Proof-of-concept
Successfully loaded the following locally in notebook:
As you can see, JWST filenames are a little long, so I need to scroll to get the full screenshot(s).
TODO
remote_data
and stuff from PO.)