Update Mosviz parser to load level 2 data #1835

rosteen · 2022-11-10T19:06:04Z

Does what it says on the tin. In-progress, currently (mostly) working for NIRSpec level 2.

EDIT: ready for review, now also loads NIRCam data. I have NIRCam test data, I can upload that to Box (I trimmed a couple real files from ~1200 sources down to 20 to speed things up) and add a test.

pllim · 2022-11-16T20:04:52Z

Need a change log. Thanks!

rosteen · 2022-11-16T20:07:41Z

Need a change log. Thanks!

I know! I thought we agreed that can wait until after approvals in case the PR drags on with lots of changes and conflicts in the changelog happen 😅

rosteen · 2022-11-16T20:08:46Z

The NIRCam test data download link is here . Working on uploading the level 2 NIRSpec data I've been using to Box as well.

codecov · 2022-11-16T21:04:13Z

Codecov Report

Base: 88.43% // Head: 88.46% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (30452e4) compared to base (e62105b).
Patch coverage: 89.89% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1835      +/-   ##
==========================================
+ Coverage   88.43%   88.46%   +0.02%     
==========================================
  Files          95       95              
  Lines       10487    10529      +42     
==========================================
+ Hits         9274     9314      +40     
- Misses       1213     1215       +2

Impacted Files	Coverage Δ
jdaviz/configs/mosviz/plugins/parsers.py	`90.92% <89.36%> (-0.21%)`	⬇️
jdaviz/configs/mosviz/helper.py	`87.24% <100.00%> (-0.12%)`	⬇️
jdaviz/app.py	`94.57% <0.00%> (+0.42%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

rosteen · 2022-11-17T16:15:55Z

A sample NIRSpec level 2 dataset for testing is now on Box as well.

pllim

Just a general review.

jdaviz/configs/mosviz/helper.py

jdaviz/configs/mosviz/plugins/parsers.py

pllim · 2022-11-18T20:52:37Z

jdaviz/configs/mosviz/plugins/parsers.py

-        im_split = image_file.stem.split("_")[0]
-        pupil = fits.getheader(image_file, ext=0).get('PUPIL')
+    if "Direct Image" in files_by_labels:
+        print("Loading: Images")


I never liked printing stuff out to the notebook. While you are at this, should we move this to the snackbar info?

pllim · 2022-11-18T20:53:19Z

jdaviz/configs/mosviz/plugins/parsers.py

+                data_iter = get_image_data_iterator(app, temp, "Image", ext=None)
+                data_obj = [d[0] for d in data_iter]  # We do not use the generated labels
+                image_data = data_obj[0]  # Grab the first one.
+                # TODO: Error if multiple found?


Is this TODO for this PR or future work?

Great question - I think I should do it here (or alternatively at least think about it and decide it's not needed). I can do that after meetings today.

jdaviz/configs/mosviz/plugins/parsers.py

jdaviz/configs/mosviz/tests/test_parsers.py

javerbukh · 2022-11-21T14:39:02Z

Was able to get NIRCAM and NIRSpec level 2 data to load, nice work!! I can approve once CI passes or if local CI passes (assuming that astropy CI bug is ongoing).

pllim · 2022-11-21T14:40:18Z

Nice try but change log is not astropy's fault. 😆

rosteen · 2022-11-21T20:36:55Z

Docs built locally:

pllim · 2022-11-21T21:38:28Z

@rosteen , if you rebase again, doc should pass.

pllim · 2022-11-21T22:27:54Z

docs/mosviz/import_data.rst

 * JWST NIRISS
+* JWST NIRCam


Why only NIRSpec has (levels 2 and 3) specified? What about NIRISS and NIRCam?

NIRISS and NIRCam only have level 2 available from what I've been told, there is no level 3.

docs/mosviz/import_data.rst

pllim · 2022-11-21T22:31:22Z

docs/mosviz/import_data.rst

@@ -50,8 +67,6 @@ and for NIRISS:

    jdaviz mosviz /path/to/my/data --instrument=niriss

-If an instrument is not specified in either case, Mosviz will default to NIRSpec parsing.


I think we want to keep this sentence but tell the user now that it would throw an error.

pllim · 2022-11-21T22:37:59Z

jdaviz/configs/mosviz/tests/test_parsers.py

            mosviz_helper.load_data(directory=data_dir)

+        return


It feels unnatural to return in a test function. Maybe this is a sign that parametrize is not suitable for this test case anymore. You don't need remote data to prove that the function will throw an exception, right? Theoretically, you could give a dummy file name/object and it would still throw the same exception?

Anyways, if we want to chuck this in the technical debt bin, we can refactor in the future and leave this as-is.

Into the bin!

rosteen · 2022-11-22T16:18:48Z

@javerbukh Tests passed, mind approving?

pllim · 2022-11-30T21:02:19Z

If you rebase, CI should be all green. FYI.

… files

… correct

Fix indent

Codestyle Add more checks to NIRCam test

Codestyle

Co-authored-by: P. L. Lim <[email protected]>

pllim

Code looks like code. Thanks!

github-actions bot added the mosviz label Nov 10, 2022

rosteen force-pushed the mosviz-load-level-2 branch 2 times, most recently from afc8112 to 8b09ea6 Compare November 15, 2022 20:09

rosteen marked this pull request as ready for review November 16, 2022 19:59

rosteen requested review from duytnguyendtn, javerbukh, ojustino, pllim, kecnry and bmorris3 as code owners November 16, 2022 19:59

pllim requested a review from eteq November 16, 2022 20:04

pllim added this to the 3.2 milestone Nov 16, 2022

rosteen force-pushed the mosviz-load-level-2 branch from 6e1b18a to 6432696 Compare November 17, 2022 21:04

pllim reviewed Nov 18, 2022

View reviewed changes

rosteen force-pushed the mosviz-load-level-2 branch from 5df9b00 to c3b93ca Compare November 21, 2022 21:44

pllim reviewed Nov 21, 2022

View reviewed changes

javerbukh approved these changes Nov 22, 2022

View reviewed changes

rosteen force-pushed the mosviz-load-level-2 branch from 9ff3b46 to bb3951c Compare November 29, 2022 21:22

Mosviz parser now handles NIRSpec directory with multi-object level 2…

daa1c8c

… files

rosteen and others added 18 commits November 30, 2022 16:07

Generalizing this for NIRCam

139a128

Remove debugging prints

8479431

Moving towards unification....

cb06a71

NIRCam loading no longer errors out, needs confirmation that it's all…

963898b

… correct

More generalization, debugging NIRCam load

dc45009

De-duplicate source list from HDUS

e4c7c17

Fix codestyle and one test failure

85f5db1

Fix failing tests

0832073

Fix indent

Update docstring and add NIRCam directory parsing test

9085f91

Codestyle Add more checks to NIRCam test

Add simple NIRSpec level 2 loading test

95cab5f

Codestyle

Correct instrument for test

a8e8b32

Apply suggestions from code review

d8c2e40

Co-authored-by: P. L. Lim <[email protected]>

Address other review comments.

91f2b95

Add documentation, changelog, and fix codestyle

b459019

Codestyle

c343c89

Apply suggestions from code review

a76f036

Co-authored-by: P. L. Lim <[email protected]>

Add clarifying comment

bdbc3b9

Add back note about instrument requirement

30452e4

rosteen force-pushed the mosviz-load-level-2 branch from bb3951c to 30452e4 Compare November 30, 2022 21:07

pllim approved these changes Nov 30, 2022

View reviewed changes

pllim merged commit dcfd9f1 into spacetelescope:main Nov 30, 2022

Jdaviz-Triage-Bot mentioned this pull request Dec 1, 2022

Parsers to emit their own error messages #1888

Closed

kecnry mentioned this pull request Dec 20, 2022

do not interpret return from parsers as message #1941

Merged

9 tasks

duytnguyendtn mentioned this pull request Jan 10, 2023

Mosviz Retrieve SourceID from meta #1851

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Mosviz parser to load level 2 data #1835

Update Mosviz parser to load level 2 data #1835

rosteen commented Nov 10, 2022 •

edited

Loading

pllim commented Nov 16, 2022

rosteen commented Nov 16, 2022

rosteen commented Nov 16, 2022

codecov bot commented Nov 16, 2022 •

edited

Loading

rosteen commented Nov 17, 2022

pllim left a comment

pllim Nov 18, 2022

pllim Nov 18, 2022

rosteen Nov 21, 2022

javerbukh commented Nov 21, 2022

pllim commented Nov 21, 2022

rosteen commented Nov 21, 2022

pllim commented Nov 21, 2022

pllim Nov 21, 2022

rosteen Nov 22, 2022

pllim Nov 21, 2022

pllim Nov 21, 2022

rosteen Nov 22, 2022

rosteen commented Nov 22, 2022

pllim commented Nov 30, 2022

pllim left a comment

		@@ -50,8 +67,6 @@ and for NIRISS:

		jdaviz mosviz /path/to/my/data --instrument=niriss

		If an instrument is not specified in either case, Mosviz will default to NIRSpec parsing.

Update Mosviz parser to load level 2 data #1835

Update Mosviz parser to load level 2 data #1835

Conversation

rosteen commented Nov 10, 2022 • edited Loading

pllim commented Nov 16, 2022

rosteen commented Nov 16, 2022

rosteen commented Nov 16, 2022

codecov bot commented Nov 16, 2022 • edited Loading

Codecov Report

rosteen commented Nov 17, 2022

pllim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javerbukh commented Nov 21, 2022

pllim commented Nov 21, 2022

rosteen commented Nov 21, 2022

pllim commented Nov 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosteen commented Nov 22, 2022

pllim commented Nov 30, 2022

pllim left a comment

Choose a reason for hiding this comment

rosteen commented Nov 10, 2022 •

edited

Loading

codecov bot commented Nov 16, 2022 •

edited

Loading