Aperture photometry in Cubeviz #2666

pllim · 2024-01-18T23:09:43Z

Description

This pull request is to unleash Imviz aperture photometry on Cubeviz.

I need:

to wait for Aperture live-preview plugin marks #2664
Kyle to tell me how to filter out undesirable things from data selection menu. Probably don't want ~~VAR and~~ MASK cubes. self.dataset.add_filter(my_filter_callable)
Ricky to tell me if it makes sense to allow photometry on moment map outputs. Cami/Kyle: Check unit physical type and let flux ones through. Does not matter if it is VAR. Make sure it works in slice and collapsed.
Someone to tell me if there is any other feature in Cubeviz I need to safeguard against: unit conversion, line analysis, and what-nots?
Cami or Patrick to give me a real use case and expected answers so I can make sure this is scientifically viable. Preferably the target should be off-center so I can make sure all the X/Y/Z transposing done by specutils is not tripping us up. Maybe try https://spacetelescope.github.io/jdat_notebooks/notebooks/ifu_optimal/ifu_optimal.html

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?
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)? 🐱 🐱 🐱

jdaviz/configs/cubeviz/helper.py

jdaviz/configs/cubeviz/plugins/moment_maps/moment_maps.py

jdaviz/configs/cubeviz/plugins/tests/test_cubeviz_aperphot.py

pllim · 2024-01-27T06:02:57Z

jdaviz/configs/cubeviz/plugins/tests/test_parsers.py

@@ -53,7 +53,7 @@ def test_fits_image_hdu_with_microns(image_cube_hdu_obj_microns, cubeviz_helper)
        flux_unit_str = "erg / (Angstrom cm2 s)"
    else:
        flux_unit_str = "erg / (Angstrom s cm2)"
-    assert label_mouseover.as_text() == (f'Pixel x=00.0 y=00.0 Value +1.00000e+00 1e-17 {flux_unit_str}',  # noqa
+    assert label_mouseover.as_text() == (f'Pixel x=00.0 y=00.0 Value +5.00000e+00 1e-17 {flux_unit_str}',  # noqa


I modified the test data so I can use it for photometry test. This change is expected.

pllim · 2024-01-27T06:03:57Z

jdaviz/configs/default/plugins/collapse/collapse.py

I need to attach WCS to collapsed data for consistent aperture photometry behavior. If this is somehow yet another ticket, please let me know.

pllim · 2024-01-27T06:04:34Z

jdaviz/configs/default/plugins/collapse/collapse.py

@@ -170,7 +180,7 @@ def _save_collapsed_spec_to_fits(self, overwrite=False, *args):
                return

        filename = str(filename)
-        CCDData(self.collapsed_spec).write(filename)


Weird that you don't assign CCDData at creation but only at writing it out. I fixed it.

pllim · 2024-01-27T06:05:19Z

jdaviz/configs/default/plugins/gaussian_smooth/gaussian_smooth.py

Again, I needed to fix the WCS here for consistent photometry behavior.

pllim · 2024-01-27T06:07:14Z

jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.py

        if hasattr(reg, 'to_pixel'):
            sky_center = reg.center
-            xcenter, ycenter = data.coords.world_to_pixel(sky_center)
+            if self.config == "cubeviz" and data.ndim > 2:
+                ycenter, xcenter = w.world_to_pixel(


Note: I tried to be lazy here, i.e., extracting just the wcs.celestial component and then use it without wavelength input, but the answer came out wrong, probably because of the specutils transpose, so let's just do it the hard way here.

jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.vue

pllim · 2024-01-27T06:09:22Z

notebooks/concepts/cubeviz_aperture_photometry.ipynb

If you want to run the new tests I wrote interactively, they are identical to the ones I put in this notebook. Feel free to use this for review if you don't have anything better.

codecov · 2024-01-27T06:30:31Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (62231d8) 90.84% compared to head (60a564b) 90.92%.

❗ Current head 60a564b differs from pull request most recent head bbb5fad. Consider uploading reports for the commit bbb5fad to get more accurate results

Files	Patch %	Lines
...imviz/plugins/aper_phot_simple/aper_phot_simple.py	94.73%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2666      +/-   ##
==========================================
+ Coverage   90.84%   90.92%   +0.07%     
==========================================
  Files         162      163       +1     
  Lines       21140    21356     +216     
==========================================
+ Hits        19205    19418     +213     
- Misses       1935     1938       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kecnry

Seems to work quite well and thanks for the thorough test coverage of different input cases!

For cube/slice support, we might want:

either a read-only indication of the slice being used, a message that the current slice is used, or an input that is two-way synced to the slider?
a column in the resulting output table that stores the slice number?

jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.vue

jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.py

pllim · 2024-01-30T18:21:34Z

I think I have addressed all the comments, @kecnry and @camipacifici .

kecnry · 2024-01-30T18:45:12Z

The table shows the slice in wavelength, but rounded to an integer wavelength which is not terribly helpful. Can we use the precision needed based on the resolution of the spectrum?

The read-only slice indication at the top of the plugin still shows slice index. If we're using wavelength in the table, perhaps we should here as well?

kecnry · 2024-01-30T19:19:10Z

What controls the output format of the GUI table?

See here - this already has some rules for slice expecting an integer.... so maybe we shouldn't call this slice or will need extra rules here for this case (checking unit as well or something like that).

Maybe we even want to just adhere to item.info.format? 🤷‍♂️

pllim · 2024-01-30T20:25:46Z

Precision on GUI table is fixed. And replace slice index with wavelength on GUI as well.

Good now?

kecnry · 2024-01-31T13:19:13Z

jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.py

+                slice_val = self._cube_wave
+            else:
+                slice_val = u.Quantity(np.nan, self._cube_wave.unit)
+            phot_table.add_column(slice_val, name="slice_wave", index=29)


is this going to conflict when we add display-unit support in cubeviz that allows changing between wavelength and frequency units? We probably can defer and change the name (and possibly handle needing to move units to its own column) at that point, but we'll need a follow-up ticket/issue.

This won't be the only conflict when the unit changing stuff happens (flux conversion, anyone?). So yeah, can be dealt with later because a lot of other stuff would also need to change together with it.

(Unfortunate though because QTable is supposed to make storing Quantity easier.)

ok, since this is getting in before that, I made a ticket for handling all of that linking back here 🐱

kecnry

Just two remaining thoughts mentioned above, but probably can be dealt with after merge, so I'll approve so someone else can start reviewing. Thanks!

javerbukh

Diff looks good and plugin works well, nice! Only have two comments: it would be convenient if the slice/wavelength at which the chart was calculated was shown above it or close by and another QOL feature would be what @kecnry implemented in spectral extraction where the subset you are using is highlighted in the viewer. Both of those can be deferred to a future PR if that makes more sense though.

kecnry · 2024-02-01T21:34:18Z

where the subset you are using is highlighted in the viewer

that should be here now since that is already merged and I see it in my screenshots above. Is it not working for you?

pllim · 2024-02-01T22:08:26Z

if the slice/wavelength at which the chart was calculated was shown above it

Wavelength is there. I originally implemented slice index but people asked for wavelength. It is uneditable text field below the data drop-down. Did you not see it?

pllim · 2024-02-02T15:44:14Z

Wavelength in

Listing in text (not table)
Listing in profile plot

Add tests and documentation. Attach WCS to some Cubeviz 2D products, where it is needed but missing. Minor clean-ups.

for cubeviz

Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>

and do not display slice for cubeviz 2D data, as requested.

for Cubeviz phot table.

and display slice wavelength instead of index in read-only text field.

* Implement Cubeviz aperture photometry. Add tests and documentation. Attach WCS to some Cubeviz 2D products, where it is needed but missing. Minor clean-ups. * Add slice info to aper phot plugin for cubeviz * Clarify slice field in aper phot Co-authored-by: Kyle Conroy <kyleconroy@gmail.com> * Fix plugin data menu bug * Implement slice in output table and do not display slice for cubeviz 2D data, as requested. * Add doc for new slice column for Cubeviz phot table. * Fix table GUI for slice wavelength and display slice wavelength instead of index in read-only text field. * Display slice_wave in plot and text results --------- Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>

pllim added this to the 3.9 milestone Jan 18, 2024

github-actions bot added documentation Explanation of code and concepts cubeviz imviz plugin Label for plugins common to multiple configurations labels Jan 18, 2024

pllim mentioned this pull request Jan 19, 2024

Aperture live-preview plugin marks #2664

Merged

13 tasks

pllim commented Jan 23, 2024

View reviewed changes

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

pllim force-pushed the unleash-the-aperphot-kraken branch 4 times, most recently from 4e3fcf5 to 97f6679 Compare January 27, 2024 05:59

pllim commented Jan 27, 2024

View reviewed changes

pllim marked this pull request as ready for review January 27, 2024 06:32

pllim requested review from rosteen, javerbukh, kecnry, bmorris3, haticekaratay, cshanahan1 and gibsongreen as code owners January 27, 2024 06:32

kecnry reviewed Jan 29, 2024

View reviewed changes

pllim force-pushed the unleash-the-aperphot-kraken branch from cd2d876 to f85ec71 Compare January 30, 2024 18:11

This comment was marked as resolved.

Sign in to view

pllim force-pushed the unleash-the-aperphot-kraken branch from c7d10df to 60a564b Compare January 30, 2024 20:29

kecnry reviewed Jan 31, 2024

View reviewed changes

kecnry approved these changes Jan 31, 2024

View reviewed changes

kecnry added the Ready for final review label Jan 31, 2024

javerbukh approved these changes Feb 1, 2024

View reviewed changes

pllim and others added 8 commits February 2, 2024 12:40

Implement Cubeviz aperture photometry.

a1583ca

Add tests and documentation. Attach WCS to some Cubeviz 2D products, where it is needed but missing. Minor clean-ups.

Add slice info to aper phot plugin

54ef338

for cubeviz

Clarify slice field in aper phot

8473db2

Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>

Fix plugin data menu bug

1350f2e

Implement slice in output table

8f0dc0a

and do not display slice for cubeviz 2D data, as requested.

Add doc for new slice column

d4f4a9b

for Cubeviz phot table.

Fix table GUI for slice wavelength

890bf41

and display slice wavelength instead of index in read-only text field.

Display slice_wave in plot and text results

bbb5fad

pllim force-pushed the unleash-the-aperphot-kraken branch from 60a564b to bbb5fad Compare February 2, 2024 18:04

pllim enabled auto-merge (squash) February 2, 2024 18:09

pllim merged commit 9373a9e into spacetelescope:main Feb 2, 2024
13 checks passed

pllim deleted the unleash-the-aperphot-kraken branch February 2, 2024 18:31

pllim mentioned this pull request Feb 2, 2024

Aperture validate after selection #2684

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aperture photometry in Cubeviz #2666

Aperture photometry in Cubeviz #2666

pllim commented Jan 18, 2024 •

edited

Loading

pllim Jan 27, 2024

pllim Jan 27, 2024

pllim Jan 27, 2024

pllim Jan 27, 2024

pllim Jan 27, 2024

pllim Jan 27, 2024

codecov bot commented Jan 27, 2024 •

edited

Loading

kecnry left a comment

pllim commented Jan 30, 2024

kecnry commented Jan 30, 2024

This comment was marked as resolved.

kecnry commented Jan 30, 2024 •

edited

Loading

This comment was marked as resolved.

pllim commented Jan 30, 2024

kecnry Jan 31, 2024

pllim Jan 31, 2024

pllim Jan 31, 2024

kecnry Jan 31, 2024

kecnry left a comment

javerbukh left a comment

kecnry commented Feb 1, 2024

pllim commented Feb 1, 2024

pllim commented Feb 2, 2024

Aperture photometry in Cubeviz #2666

Aperture photometry in Cubeviz #2666

Conversation

pllim commented Jan 18, 2024 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

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

codecov bot commented Jan 27, 2024 • edited Loading

Codecov Report

kecnry left a comment

Choose a reason for hiding this comment

pllim commented Jan 30, 2024

kecnry commented Jan 30, 2024

This comment was marked as resolved.

kecnry commented Jan 30, 2024 • edited Loading

This comment was marked as resolved.

pllim commented Jan 30, 2024

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

kecnry left a comment

Choose a reason for hiding this comment

javerbukh left a comment

Choose a reason for hiding this comment

kecnry commented Feb 1, 2024

pllim commented Feb 1, 2024

pllim commented Feb 2, 2024

pllim commented Jan 18, 2024 •

edited

Loading

codecov bot commented Jan 27, 2024 •

edited

Loading

kecnry commented Jan 30, 2024 •

edited

Loading