Update Mosviz parser doc and fix NIRSpec auto-load #2146

pllim · 2023-04-11T22:08:05Z

Description

This pull request is to update Mosviz doc on importing data, and also enable NIRSpec auto loader to take a single image.

And then the scope expanded and now it also fixes #2144

Change log entry

Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
should add a no-changelog-entry-needed label.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
Do the proposed changes follow the STScI Style Guides?
Are tests added/updated as required? If so, do they follow the STScI Style Guides? Not sure how to add tests that don't require downloading a bunch of remote data files. Can we not?
Are docs added/updated as required? If so, do they follow the STScI Style Guides?
Did the CI pass? If not, are the failures related?
Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)? 🐱 🐱 🐱

codecov · 2023-04-12T01:01:47Z

Codecov Report

Patch coverage: 80.00% and project coverage change: -0.11 ⚠️

Comparison is base (10adfb6) 91.61% compared to head (0362037) 91.50%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2146      +/-   ##
==========================================
- Coverage   91.61%   91.50%   -0.11%     
==========================================
  Files         147      147              
  Lines       16098    16142      +44     
==========================================
+ Hits        14748    14771      +23     
- Misses       1350     1371      +21

Impacted Files	Coverage Δ
jdaviz/configs/mosviz/tests/test_parsers.py	`99.09% <ø> (-0.04%)`	⬇️
jdaviz/configs/mosviz/plugins/viewers.py	`87.58% <52.94%> (-1.31%)`	⬇️
jdaviz/configs/mosviz/plugins/parsers.py	`89.27% <73.33%> (-0.56%)`	⬇️
jdaviz/configs/mosviz/helper.py	`85.11% <82.75%> (-2.25%)`	⬇️
.../configs/cubeviz/plugins/slice/tests/test_slice.py	`100.00% <100.00%> (ø)`
jdaviz/configs/mosviz/tests/test_data_loading.py	`100.00% <100.00%> (ø)`
jdaviz/configs/mosviz/tests/test_helper.py	`100.00% <100.00%> (ø)`

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

camipacifici · 2023-04-12T16:25:23Z

Thank you very much. This fixes my original problem.
I see three additional problems here:

the slit overlay is likely wrong (and this is probably not a jdaviz problem...lets just ignore this for now)
the table is returning Unspecified for all the sources under the RA and Dec columns
the position where the image viewer is zooming to is different from the position given by the SRCRA and SRCDEC keywords. I am not 100% positive that these keywords are the right ones, but maybe this can be double checked with the NIRISS parser that I am sure is correct.

camipacifici · 2023-04-12T17:46:24Z

Would be nice to fix the image loader for this one too:
#2144

pllim · 2023-04-12T21:03:02Z

the table is returning Unspecified for all the sources under the RA and Dec columns

For some reason, it is trying to look for RA and Dec on the image, not spectra. Not sure why. I added a fix.

the position where the image viewer is zooming to is different from the position given by the SRCRA and SRCDEC keywords

Hard for me to test using the cutout off the 2GB image you provided. If this is still a problem, please be more specific on what the correct answer should be. In NIRISS, I see that the parser grab RA and Dec off a special ECSV catalog that NIRSpec does not have, so I cannot just use the NIRISS logic. And are you sure this is not the same problem as S_REGION? These are all populated by the pipeline.

Would be nice to fix the image loader

I also added a fix for this. Please test your use case in #2144 with this PR too.

pllim · 2023-04-12T21:05:47Z

jdaviz/configs/mosviz/helper.py

        """
        super().load_data(data_obj, parser_reference="mosviz-image-parser",
                          data_labels=data_labels, share_image=share_image)
-        self._add_redshift_column()
+        if add_redshift_column:
+            self._add_redshift_column()


Note: Multiple calls to self._add_redshift_column() in load_data was what causing all the warnings Cami was seeing, so I have to disable this call in sub-calls by default to get rid of the warnings.

I confirm I do not see the warnings anymore.

jdaviz/configs/mosviz/plugins/parsers.py

camipacifici · 2023-04-13T02:11:56Z

The table is nicely populated, but if I try to load the image I still get the same traceback that ends with
ValueError: The dimensions of component 2D Spectra are incompatible with the dimensions of this data: (50,) vs (1,)

This is the way I am calling it:
mosviz = Mosviz()
mosviz.load_data(spectra_1d="../data/mastDownload/jones_calx1d_files/single/jw02136001001_03137_00001_nrs1_x1d.fits", spectra_2d="../data/mastDownload/jones_s2d_level2/jw02136001001_03137_00001_nrs1_s2d.fits", images="/Users/cpacifici/Dropbox/SINCRONIZZARE/Documents/WORK/CANDELS/hlsp_candels_hst_wfc3_egs-tot-60mas_f160w_v1.0_drz.fits", instrument="nirspec")
mosviz.show()

pllim · 2023-04-13T15:21:39Z

There are just too many ways to do the same thing in Mosviz. Looks like you were using a different logic route. I have pushed a fix.

By the way, you don't have to specify instrument= unless you are passing in a directory=, otherwise it does nothing and just more typing for you.

pllim · 2023-04-13T15:40:42Z

jdaviz/configs/mosviz/tests/test_data_loading.py

@@ -212,7 +209,7 @@ def test_load_single_image_multi_spec(mosviz_helper, mos_image, spectrum1d, mos_
    label_mouseover._viewer_mouse_event(spec2d_viewer,
                                        {'event': 'mousemove', 'domain': {'x': 10, 'y': 100}})
    assert label_mouseover.as_text() == ('Pixel x=00010.0 y=00100.0 Value +8.12986e-01', '', '')
-    assert label_mouseover.icon == 'b'
+    assert label_mouseover.icon == 'c'


Note: I changed the loading order in load_data to fix a bug, hence this change here.

camipacifici · 2023-04-13T16:29:52Z

The way I was using now works, thank you!
Out of curiosity, what was the way that worked for you before this last fix?

Now, I see another problem. The image viewer is not zooming on SRCRA and SRCDEC (it is quite far off). Do you see the same on your side? I can demo with the big image if you wish. The same is valid for the slit overlay. It is off from the coordinates in S_REGION.

pllim · 2023-04-13T16:40:47Z

what was the way that worked for you

I was loading using directory, having moved the image under some sub-dir that it looks for. I did not try to load all 3 components separately and when I did, I could reproduce your problem (and then fix it).

camipacifici · 2023-04-13T16:42:50Z

Understood. Let me try to find a different image!

pllim · 2023-04-13T16:53:33Z

Hmm I just found the code that zoom on shared image. I forgot this is not Imviz, so maybe that is a real bug, lemme see.

camipacifici · 2023-04-17T17:32:59Z

You are right!! My mistake, the image is there and the zoom is doing what it is supposed to, i.e. zooming using the number of pixels in the cross dispersion direction in the 2D image. That is apparently too small, but it not a jdaviz problem. Also the fact that the RA/Dec are not in the header is not a jdaviz problem.

For the record, I did not have to put the image under an images directory. I just included it in the folder, like for NIRISS, so docs are also correct.

Sorry for the false alarm! This looks good to me!

pllim · 2023-04-17T20:23:26Z

Failures are unrelated.

rosteen · 2023-04-18T17:34:23Z

I get a test failure that appears related when I run them locally, from test_nirspec_parser[nirspec]:

>       assert len(mosviz_helper.app.data_collection) == 16
E       AssertionError: assert 11 == 16
E        +  where 11 = len(DataCollection (11 data sets)\n	  0: MOS Table\n	  1: 1D Spectrum 0\n	  2: 1D Spectrum 1\n	  3: 1D Spectrum 2\n	  4: 1D Spe...	  5: 1D Spectrum 4\n	  6: 2D Spectrum 0\n	  7: 2D Spectrum 1\n	  8: 2D Spectrum 2\n	  9: 2D Spectrum 3\n	 10: 2D Spectrum 4)
E        +    where DataCollection (11 data sets)\n	  0: MOS Table\n	  1: 1D Spectrum 0\n	  2: 1D Spectrum 1\n	  3: 1D Spectrum 2\n	  4: 1D Spe...	  5: 1D Spectrum 4\n	  6: 2D Spectrum 0\n	  7: 2D Spectrum 1\n	  8: 2D Spectrum 2\n	  9: 2D Spectrum 3\n	 10: 2D Spectrum 4 = Application(config='mosviz', events=['call_viewer_method', 'close_snackbar_message', 'data_item_remove', 'data_item_un...erflow hidden will prevent the scrollbar from\n   * appearing */\n  overflow: hidden;\n}\n</style>\n'), vdocs='latest').data_collection
E        +      where Application(config='mosviz', events=['call_viewer_method', 'close_snackbar_message', 'data_item_remove', 'data_item_un...erflow hidden will prevent the scrollbar from\n   * appearing */\n  overflow: hidden;\n}\n</style>\n'), vdocs='latest') = <jdaviz.configs.mosviz.helper.Mosviz object at 0x13f810550>.app

configs/mosviz/tests/test_parsers.py:38: AssertionError

I assume that you weren't seeing this locally, any idea what might be going wrong?

pllim · 2023-04-18T17:48:25Z

@rosteen , I seen that before. I think some data was not loaded properly. I thought I fixed that. Are you testing an older version of my branch? Did you try clearing the download cache?

rosteen · 2023-04-18T18:01:33Z

The test passed after I used astropy's clear_download_cache, good call. I'll try to finish reading through the code changes soon.

rosteen

I left a couple small questions and one small suggestion, but otherwise I think this looks good.

docs/mosviz/import_data.rst

rosteen · 2023-04-18T18:32:08Z

jdaviz/configs/mosviz/plugins/viewers.py

-    def set_plot_axes(self):
-        self.figure.axes[1].tick_format = None
-        self.figure.axes[0].tick_format = None
+    def _mark_targets(self):


Where does this get used, anyway? This is the only instance of the string "_mark_targets" that I see in the repository.

I needed it to debug. I wanted to see where the RA and Dec from the table actually are. I feel like we could make it public (in one way or another) in the future but I don't want that headache right now.

It comes "for free" with the Astrowidgets inheritance, but if this bothers you, I can remove it.

Ah, I see. I don't think it needs to be removed then, but it would be nice to have a comment saying that it's there to help with debugging and isn't actually called internally.

I could see this being useful eventually, why not 🤷

Comment added.

rosteen · 2023-04-18T18:32:52Z

jdaviz/configs/mosviz/plugins/viewers.py

+        from astropy.coordinates import SkyCoord
+        from astropy.table import QTable


Depends on the answer to the question above probably, but maybe move these to the top level? I generally dislike imports in functions.

Does #2146 (comment) help you making a decision on this?

If it's only used in debugging it can stay.

The comment doesn't quite give me an answer as to why these imports should be down here. Is there a reason?

Because the imports are unnecessary otherwise. If we decide to delete this method one day, it gives a cleaner diff. If you really want the imports to be high up, I can do it too. Lemme know.

Understandable, but isn't this directly against PEP8? https://peps.python.org/pep-0008/#imports

Imports are always put at the top of the file, just after any module comments and docstrings, and before module globals and constants.

Unless those particular imports take a particularly long time, I'd argue following PEP8. I'm personally comfortable with counting this as a value-add rather than a "small debugging thing that might be useful later"

Imports moved up.

Add change log. Ignore non-FITS files in NIRSpec glob. Grab source RA/Dec from 1D spec first.

remove unnecessary ignoring of warnings that are not there.

Update change log

and disable image viewer axes

Co-authored-by: Ricky O'Steen <39831871+rosteen@users.noreply.github.com>

that it is only for debugging

duytnguyendtn

For whatever reason, I couldn't get to Cami's nirspec example, but the nircam example worked and zoomed well. Code looks good and much more logical. Thanks Pey-Lian! A few other comments

jdaviz/configs/mosviz/helper.py

and rename xh variable

so we just blindly ignore, not caring if it shows up or not

pllim · 2023-04-18T22:46:05Z

@rosteen and @duytnguyendtn , is this okay to merge now? Thanks for the reviews!

* Update Mosviz parser doc and fix NIRSpec auto-load. Add change log. Ignore non-FITS files in NIRSpec glob. Grab source RA/Dec from 1D spec first. * Update Mosviz example notebook * Update tests and remove unnecessary ignoring of warnings that are not there. * Fix single image loading for mix-and-match Update change log * Fix Mosviz image zoom to target and disable image viewer axes * BUG: images not returning Path objects * Fix English typo Co-authored-by: Ricky O'Steen <39831871+rosteen@users.noreply.github.com> * Add a note about _mark_targets that it is only for debugging * Move local imports up and rename xh variable * TST: Warning gone from test_slice * OMG now warning is back so we just blindly ignore, not caring if it shows up or not --------- Co-authored-by: Ricky O'Steen <39831871+rosteen@users.noreply.github.com>

pllim added the documentation Explanation of code and concepts label Apr 11, 2023

pllim added this to the 3.5 milestone Apr 11, 2023

github-actions bot added the mosviz label Apr 11, 2023

pllim marked this pull request as ready for review April 12, 2023 01:04

pllim requested review from duytnguyendtn, rosteen, javerbukh, kecnry, bmorris3, haticekaratay and cshanahan1 as code owners April 12, 2023 01:04

pllim force-pushed the mosviz-loading-doc branch from 17b9b5e to c3f00ab Compare April 12, 2023 20:54

pllim commented Apr 12, 2023

View reviewed changes

jdaviz/configs/mosviz/plugins/parsers.py Show resolved Hide resolved

pllim added bug Something isn't working feature Feature request labels Apr 12, 2023

pllim force-pushed the mosviz-loading-doc branch from c3f00ab to bfa09bb Compare April 13, 2023 15:20

pllim force-pushed the mosviz-loading-doc branch from bfa09bb to 334b743 Compare April 13, 2023 15:26

pllim commented Apr 13, 2023

View reviewed changes

This comment was marked as resolved.

# to view

pllim force-pushed the mosviz-loading-doc branch from 80bbca5 to 07f8025 Compare April 17, 2023 19:33

kecnry removed request for kecnry, bmorris3, javerbukh, cshanahan1 and haticekaratay April 18, 2023 17:39

rosteen approved these changes Apr 18, 2023

View reviewed changes

pllim and others added 8 commits April 18, 2023 16:29

Update Mosviz parser doc and fix NIRSpec auto-load.

650d0b3

Add change log. Ignore non-FITS files in NIRSpec glob. Grab source RA/Dec from 1D spec first.

Update Mosviz example notebook

4f25fdd

Update tests and

8542eee

remove unnecessary ignoring of warnings that are not there.

Fix single image loading for mix-and-match

c4da906

Update change log

Fix Mosviz image zoom to target

1836b8b

and disable image viewer axes

BUG: images not returning Path objects

2efa57c

Fix English typo

f2b4b37

Co-authored-by: Ricky O'Steen <39831871+rosteen@users.noreply.github.com>

Add a note about _mark_targets

50e2dd1

that it is only for debugging

pllim force-pushed the mosviz-loading-doc branch from 9500a3c to 50e2dd1 Compare April 18, 2023 20:31

duytnguyendtn approved these changes Apr 18, 2023

View reviewed changes

jdaviz/configs/mosviz/helper.py Show resolved Hide resolved

jdaviz/configs/mosviz/helper.py Outdated Show resolved Hide resolved

Move local imports up

485200c

and rename xh variable

This comment was marked as resolved.

# to view

pllim added 2 commits April 18, 2023 17:29

TST: Warning gone from test_slice

c36b7b9

OMG now warning is back

0362037

so we just blindly ignore, not caring if it shows up or not

pllim merged commit c0e2ed5 into spacetelescope:main Apr 19, 2023

pllim deleted the mosviz-loading-doc branch April 19, 2023 14:50

		from astropy.coordinates import SkyCoord
		from astropy.table import QTable

Update Mosviz parser doc and fix NIRSpec auto-load #2146

Update Mosviz parser doc and fix NIRSpec auto-load #2146

Conversation

pllim commented Apr 11, 2023 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

codecov bot commented Apr 12, 2023 • edited Loading

Codecov Report

camipacifici commented Apr 12, 2023

camipacifici commented Apr 12, 2023

pllim commented Apr 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camipacifici commented Apr 13, 2023

pllim commented Apr 13, 2023 • edited Loading

Choose a reason for hiding this comment

camipacifici commented Apr 13, 2023

This comment was marked as resolved.

pllim commented Apr 13, 2023

camipacifici commented Apr 13, 2023

pllim commented Apr 13, 2023

camipacifici commented Apr 17, 2023

pllim commented Apr 17, 2023

rosteen commented Apr 18, 2023

pllim commented Apr 18, 2023

rosteen commented Apr 18, 2023

rosteen 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

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

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

duytnguyendtn left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

pllim commented Apr 18, 2023

pllim commented Apr 11, 2023 •

edited

Loading

codecov bot commented Apr 12, 2023 •

edited

Loading

pllim commented Apr 13, 2023 •

edited

Loading