ENH: Cubeviz metadata viewer #1325

pllim · 2022-05-17T16:08:05Z

Description

This pull request is to parse metadata for Cubeviz and show it in Metadata Viewer plugin.

When there is primary header:

Fixes #1324

TODO

Decide what to do with Primary header in multi-extension FITS.
Add some metadata parsing checks to parser tests.

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?
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 change log needed? If yes, is it added to CHANGES.rst?
Is a milestone set?
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

codecov · 2022-05-17T16:22:23Z

Codecov Report

Merging #1325 (07270be) into main (806c94d) will increase coverage by 1.02%.
The diff coverage is 85.60%.

@@            Coverage Diff             @@
##             main    #1325      +/-   ##
==========================================
+ Coverage   83.75%   84.77%   +1.02%     
==========================================
  Files          91       91              
  Lines        7761     7836      +75     
==========================================
+ Hits         6500     6643     +143     
+ Misses       1261     1193      -68

Impacted Files	Coverage Δ
jdaviz/configs/cubeviz/plugins/parsers.py	`56.29% <55.00%> (-1.57%)`	⬇️
jdaviz/configs/mosviz/plugins/parsers.py	`90.42% <82.35%> (-0.57%)`	⬇️
...default/plugins/metadata_viewer/metadata_viewer.py	`93.40% <88.67%> (-0.83%)`	⬇️
...imviz/plugins/aper_phot_simple/aper_phot_simple.py	`91.55% <100.00%> (+0.08%)`	⬆️
jdaviz/configs/imviz/plugins/parsers.py	`100.00% <100.00%> (ø)`
jdaviz/configs/specviz/plugins/parsers.py	`89.02% <100.00%> (+0.41%)`	⬆️
jdaviz/configs/specviz2d/plugins/parsers.py	`87.09% <100.00%> (+50.05%)`	⬆️
jdaviz/utils.py	`92.10% <100.00%> (+1.78%)`	⬆️
...configs/default/plugins/data_tools/file_chooser.py	`69.14% <0.00%> (+3.42%)`	⬆️
...s/default/plugins/model_fitting/fitting_backend.py	`93.02% <0.00%> (+15.11%)`	⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 806c94d...07270be. Read the comment docs.

pllim · 2022-05-17T21:07:10Z

jdaviz/configs/default/plugins/metadata_viewer/metadata_viewer.py

-        for badkey in ('COMMENT', 'HISTORY', ''):
+        # Some FITS keywords cause "# ipykernel cannot clean for JSON" messages.
+        # Also, we want to hide internal metadata that starts with underscore.
+        badkeys = ['COMMENT', 'HISTORY', ''] + [k for k in d if k.startswith('_')]


@kecnry , I think we talked about automatically hiding "hidden" metadata? This would do it for you.

pllim · 2022-05-17T21:13:42Z

jdaviz/configs/mosviz/plugins/parsers.py

-                    'S_REGION' not in data.meta):
-                data.meta['S_REGION'] = data.meta['header']['S_REGION']
+            # Make metadata layout conform with other viz.
+            if 'header' in data.meta:


With this change, we should no longer worry about meta[somekey] vs meta['header'][somekey].

It seems like the meta['header'] stuff was imposed by specutils, so in the future if we add new parsing logic that loads stuff from specutils, we will need to remember to do this along with the new code. A little messy but seems to work. 🤷

Awesome! With this change, the WCS logic for detecting data-type should also probably be updated (either in #1313 if this is merged first, or here if #1313 is merged first).

pllim · 2022-05-18T01:05:11Z

jdaviz/configs/mosviz/plugins/parsers.py

-            d.meta.update(meta)
-            data_list.append(d)
+        # We do not use the generated labels
+        data_list = [d for d, _ in get_image_data_iterator(app, hdulist, "Image", ext=None)]


Metadata stuff is already taken care of in get_image_data_iterator (Imviz parsing logic).

pllim · 2022-05-18T01:06:18Z

jdaviz/configs/mosviz/tests/test_data_loading.py

@@ -24,7 +28,7 @@ def test_load_spectrum1d(mosviz_helper, spectrum1d):

    assert isinstance(data[label], Spectrum1D)

-    with pytest.raises(TypeError):
+    with pytest.raises(AttributeError):


Exception class changed because it now crashes in metadata parsing. I don't think it matters?

havok2063

I played around with this a bit. I think the toggle is fine. I would suggest two changes.

That the UI remembers the toggled state as users select new extensions from the data dropdown. I see this more as a global toggle rather than a per extension toggle. I'd like to toggle it once for everything and leave it until I toggle it back.
Add in any header comments for each key name, value. Either as a third column or as a hover tooltip on the key name or the row.

havok2063 · 2022-05-18T15:55:30Z

Fair points.

That complicates the logic a little, as not all data would have primary header but I'll see what I can do.

Ahh yeah. I guess in those cases you could fall back to the existing header and de-toggle with a notice, similar to what happens now.

This is a tough one. I don't think astropy.io.fits or Glue can preserve it natively because the metadata becomes key/value paired dictionary. It is not preserved by specutils either when it parses spectra from file. So, I am not sure if this is a realistic request for this PR.

You could time box it and see how hard it is. I think it'd be useful for users to have some sort of description of what the header keys are. I imagine at some point before the data get parsed and passed into the machinery hole, the main HDUList is accessible to extract out the Header cards. But yeah, if it's not easy to pull that out, then it can be out of scope for this PR.

Also enable metadata viewer for Specviz2d

Have show_primary be more sticky.

pllim · 2022-05-18T21:32:07Z

Well, this is more code than I thought I would write for this feature but I think I have addressed your feedback, @havok2063 .

javerbukh

Tested with all the configurations and works great, nice work!

kecnry

Never expected extending the metadata viewer to cubeviz would get this... messy... but its definitely nice to have it across all viztools now. Just a few minor thoughts that you can take or leave, but otherwise I'm happy with merging this and we can iterate on any tweaks as people use it for real. Thanks!

kecnry · 2022-05-19T13:10:36Z

jdaviz/configs/cubeviz/plugins/parsers.py

+    metadata = standardize_metadata(hdulist[ext].header)
+    metadata.update(wcs_dict)  # To be internally consistent
+    if hdulist[ext].name != 'PRIMARY' and 'PRIMARY' in hdulist:
+        metadata[PRIHDR_KEY] = standardize_metadata(hdulist['PRIMARY'].header)


this seems to be a re-used block of code in a few places... could this logic be moved entirely into standardize_metadata or no?

Not really, as passing in hdulist into standardize_metadata would make it too specific to FITS, which should stay in the realm of parsers.

I mean standardize_metadata already checks for fits.Header but you could embed fits.Header in a plain dict, which is what specutils does, but one would/should never put the whole HDUList inside metadata.

kecnry · 2022-05-19T13:13:09Z

jdaviz/configs/default/plugins/metadata_viewer/metadata_viewer.vue

+    <v-row v-if="has_primary">
+      <v-switch
+        label="Show primary header"
+        hint="Show MEF primary header metadata instead."
+        v-model="show_primary"
+        persistent-hint>
+      </v-switch>
+    </v-row>


I'm not sure this is the most-intuitive.... but that can be covered in docs or iterated based on user-feedback (I don't really have any suggestions except a full extension-dropdown, but that might not be any more clear).

In Ginga, we have a checkbox toggle that would mix in primary header keys into the display, together with actual header. Not that that is not messy but no one has complained over there... 🤷

kecnry · 2022-05-19T13:15:32Z

docs/cubeviz/plugins.rst

+
+This plugin allows viewing of any metadata associated with the selected data.
+


maybe worth a mention explaining the primary switch here?

havok2063 · 2022-05-19T13:24:04Z

This is looking good. My only comments are about the styling. When the plugin tray size is small, everything gets squished together and word wraps. This improves when the plugin tray is expanded out, but that takes up real estate. Could we tweak the styles here to try to mitigate some of this? Maybe reducing the font size of the values and comments? Or making the header key bold? Something to try to reduce the amount of whitespace/padding and to visually distinguish a header key with the comment underneath when the plugin tray is small. Jenn might have some thoughts for this.

pllim · 2022-05-19T15:16:54Z

I am really afraid this is going to drag on and then go stale, so I am going to merge and address doc and styling in a follow-up PR (see #1331). Thanks!

now that spacetelescope#1325 is merged and rebased Co-authored-by: P. L. Lim <[email protected]>

pllim added this to the 2.6 milestone May 17, 2022

github-actions bot added the cubeviz label May 17, 2022

pllim commented May 17, 2022

View reviewed changes

pllim commented May 18, 2022

View reviewed changes

pllim marked this pull request as ready for review May 18, 2022 01:22

pllim requested review from duytnguyendtn, rosteen, javerbukh, ojustino and kecnry as code owners May 18, 2022 01:22

pllim requested a review from havok2063 May 18, 2022 01:22

havok2063 reviewed May 18, 2022

View reviewed changes

This comment was marked as resolved.

Sign in to view

pllim added 6 commits May 18, 2022 14:04

ENH: Cubeviz metadata viewer

25566ee

DOC: Add doc for Cubeviz metadata viewer

0ea9c8c

Separate out primary header metadata.

6236cc9

Also enable metadata viewer for Specviz2d

Add metadata viewer tests

7433fa3

Test metadata loading in all the viz

d8f0977

Inherit FITS header comment cards.

17f542c

Have show_primary be more sticky.

pllim force-pushed the cubeviz-metadata branch from b557f49 to 17f542c Compare May 18, 2022 21:04

Do not set traitlet until the end

07270be

javerbukh approved these changes May 19, 2022

View reviewed changes

kecnry approved these changes May 19, 2022

View reviewed changes

pllim merged commit 12d97b8 into spacetelescope:main May 19, 2022

pllim deleted the cubeviz-metadata branch May 19, 2022 15:17

This was referenced May 19, 2022

Data Dropdown Component #1313

Merged

Metadata Viewer: Doc and styling #1331

Merged

kecnry added a commit to kecnry/jdaviz that referenced this pull request May 19, 2022

simplify WCS access from metadata

44a9ed3

now that spacetelescope#1325 is merged and rebased Co-authored-by: P. L. Lim <[email protected]>

pllim mentioned this pull request May 24, 2022

BUG: Metadata viewer to ignore invalid FITS header #1342

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Cubeviz metadata viewer #1325

ENH: Cubeviz metadata viewer #1325

pllim commented May 17, 2022 •

edited

Loading

codecov bot commented May 17, 2022 •

edited

Loading

pllim May 17, 2022

pllim May 17, 2022

kecnry May 18, 2022

pllim May 18, 2022

pllim May 18, 2022

havok2063 left a comment

This comment was marked as resolved.

havok2063 commented May 18, 2022

pllim commented May 18, 2022

javerbukh left a comment

kecnry left a comment

kecnry May 19, 2022

pllim May 19, 2022

pllim May 19, 2022

kecnry May 19, 2022

pllim May 19, 2022

kecnry May 19, 2022

havok2063 commented May 19, 2022

pllim commented May 19, 2022 •

edited

Loading


		This plugin allows viewing of any metadata associated with the selected data.

ENH: Cubeviz metadata viewer #1325

ENH: Cubeviz metadata viewer #1325

Conversation

pllim commented May 17, 2022 • edited Loading

Description

TODO

Checklist for package maintainer(s)

codecov bot commented May 17, 2022 • edited Loading

Codecov Report

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

havok2063 left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

havok2063 commented May 18, 2022

pllim commented May 18, 2022

javerbukh left a comment

Choose a reason for hiding this comment

kecnry 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

havok2063 commented May 19, 2022

pllim commented May 19, 2022 • edited Loading

pllim commented May 17, 2022 •

edited

Loading

codecov bot commented May 17, 2022 •

edited

Loading

pllim commented May 19, 2022 •

edited

Loading