FEAT: Radial profile in Imviz simple aperture photometry plugin #1030

pllim · 2022-01-06T17:10:38Z

Description

This pull request is to add radial plot to simple aperture photometry plugin in Imviz.

Close #962

🐱

TODO

Replace matplotlib with bqplot. ~~Awaiting~~ Use findings from @kecnry .
Fix bqplot figure not resizing.
Ensure reasonable dark mode support.

Screenshots (bqplot)

HST/ACS data (dark mode):

JWST simulated data:

photutils example data (no WCS nor unit):

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? Milestone is only currently required for PRs related to Imviz MVP.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

codecov · 2022-01-06T17:16:12Z

Codecov Report

Attention: Patch coverage is 95.34884% with 2 lines in your changes missing coverage. Please review.

Project coverage is 74.71%. Comparing base (9d35f7f) to head (081a720).
Report is 2676 commits behind head on main.

Files	Patch %	Lines
jdaviz/app.py	60.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1030      +/-   ##
==========================================
+ Coverage   74.58%   74.71%   +0.13%     
==========================================
  Files          77       77              
  Lines        5835     5874      +39     
==========================================
+ Hits         4352     4389      +37     
- Misses       1483     1485       +2

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

pllim · 2022-01-06T17:17:53Z

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

@@ -145,6 +155,7 @@ def vue_do_aper_phot(self, *args, **kwargs):
            npix = np.sum(aper_mask) * u.pix
            img = aper_mask.get_values(comp_no_bg, mask=None)
            aper_mask_stat = reg.to_mask(mode='center')
+            comp_no_bg_cutout = aper_mask_stat.cutout(comp_no_bg)


@PatrickOgle et al. -- You do want radial profile of background subtracted data, right? If not, I can change this to plot raw data. Please confirm.

pllim · 2022-01-06T17:19:09Z

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

+                radial_img = comp_no_bg_cutout.ravel()
+                with quantity_support():
+                    fig, ax = plt.subplots()
+                    ax.plot(radial_r, radial_img, 'k.')


@larrybradley , I hope I got this right. Would be nice if you double check if I am using regions correctly for the radial plot. Thanks!

Jenneh · 2022-01-21T21:49:10Z

Here are some tweaks that will help with the UX:

Add a caption under plot that describes what the user is looking at / what the plot is. (Short description)
Add styling to table, use MAST vuetify component if possible, this will help readability by reducing spacing and adding alternating row colors

Remove some significant figures in the table results, currently there are too many
rather than sky center, add units after label, break into two rows -> RA-center and Dec center
Remove some padding so plot uses full space

Happy to discuss these on a call to further explain. Thanks!

pllim · 2022-01-21T23:11:09Z

Thanks, @Jenneh !

use MAST vuetify component if possible

I don't know what this is. Maybe @kecnry or @havok2063 can help?

Remove some padding so plot uses full space

Makes sense but hard to do with matplotlib. We are going to replace matplotlib with bqplot anyway, so I am going to hold this off for now. But I will keep this in mind.

kecnry · 2022-01-24T14:21:37Z

@pllim @Jenneh - the MAST table component is a great idea, but I would suggest we handle that across the entire app at once instead of just for this plugin.

I also agree about the padding - if bqplot ends up being a no-go, then we should fix this for matplotib, but there's no sense in doing it yet 🤞

pllim · 2022-01-24T15:02:30Z

Re: MAST vuetify component -- @Jenneh or @havok2063 , does it automatically handle dark theme? If it does not, might be non-starter.

Jenneh · 2022-01-24T15:41:53Z

We have added style overrides on the table so I think it should look the same regardless of which theme.

pllim · 2022-01-24T15:58:19Z

caption

I cannot think of a good caption. The most important info is already captured in the title above the plot and X/Y-axis labels. Do you mean I move the title from top of the plot to the bottom?

pllim · 2022-01-24T17:12:45Z

Remove some significant figures

@Jenneh , I think I already cut down on the significant figures formatting. Are you sure you are using the latest version of this feature branch?

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

javerbukh

Running this in jupyter-lab, when I press calculate my browser seems to freeze up and I get a popup asking if I want to kill the page or wait. If I press wait, eventually it finishes calculating. It would be nice to have something indicating that the calculation is ongoing, or making the radial profile optional since it seems to make the application lag once it appears.

I am out of my depths as far as the science goes but the implementation (not sure if optimization is possible/in-scope) and design looks good to me!

kecnry · 2022-02-08T13:56:46Z

@javerbukh @pllim - we could do a plugin spinner similar to #1060... but maybe that should be a separate effort (to apply across all plugins that take time and need some feedback that current shown results are out-of-date).

pllim · 2022-02-08T14:33:13Z

eventually it finishes calculating

It is not supposed to take that long. It is just basically doing simple stat along different radii from center. How big was your image and how big was the Subset?

rosteen · 2022-02-08T20:13:43Z

@javerbukh @pllim - we could do a plugin spinner similar to #1060... but maybe that should be a separate effort (to apply across all plugins that take time and need some feedback that current shown results are out-of-date).

The model fitting plugin uses a Snackbar message with a spinning symbol during cube fitting to indicate that it's running, so that's another possibility here.

pllim · 2022-02-08T20:29:17Z

Do we really need a spinner? Like I said, normal use case will only do this on a small subset area. 💭

rosteen

I have one concern about this, which is that it doesn't seem to behave well if you've linked by WCS before applying a subset. In the below screenshot I've linked by WCS, which shifts the second image to match the first (reference) image. Then, when I apply the subset to the radial calculation, it seems to be applying the subset to the unshifted data, such that the radial profile isn't properly centered. This conflicts with what the user sees in the viewer, which shows the subset centered on the star.

pllim · 2022-02-08T20:40:42Z

Oh, dear. If the radial profile is wrong, that means the photometry is wrong too, since they all use the same subset! Is this not the right way to extract Subset associated with the selected Data?

# event is the Subset label
            for lyr in viewer.layers:
                if lyr.layer.label == event and lyr.layer.data.label == self._selected_data.label:
                    subset = lyr.layer

            self._selected_subset = subset.data.get_selection_definition(
                subset_id=event, format='astropy-regions')

rosteen · 2022-02-08T21:33:30Z

I'm looking into it, but the initial thing I found is that there seem to be two "Subset 1" layers...so it might be grabbing the wrong one of the two.

>>>for lyr in viewer.layers:
>>>    print(lyr.layer.label)
acs_47tuc_1[SCI,1]
acs_47tuc_2[SCI,1]
Subset 1
Subset 1

Edit: I wrote this before realizing that your lyr.layer.data.label check might cover this.

rosteen · 2022-02-08T21:40:30Z

Actually the problem is that get_selection_definition is returning the same pixel values for both images, when they should actually have the same center in WCS but different centers in pixel space:

subset = viewer.layers[2].layer
print(subset.data.get_selection_definition(subset_id="Subset 1", 
                                           format='astropy-regions'))
subset = viewer.layers[3].layer
print(subset.data.get_selection_definition(subset_id="Subset 1", 
                                           format='astropy-regions'))

Region: CirclePixelRegion
center: PixCoord(x=1799.8775634765625, y=1141.688720703125)
radius: 33.3057861328125
Region: CirclePixelRegion
center: PixCoord(x=1799.8775634765625, y=1141.688720703125)
radius: 33.3057861328125

rosteen · 2022-02-08T21:46:59Z

This might be a good thing to delve into for our SME hack day... 😅

pllim · 2022-02-08T21:49:13Z

Oh... I remember now! I opened an issue about this a few months ago:

Subset on dithered data linked by WCS ignores the dither glue-viz/glue-astronomy#52

pllim · 2022-02-08T21:52:16Z

Well, yikes. Other than writing a known issue, I am not sure what to do here. Until the upstream bug is fixed, one should never try to do photometry on anything other than the reference data when images are dithered.

rosteen · 2022-02-08T21:53:35Z

Yeha, rough. I suspect that the root cause may be in glue rather than glue-astronomy, since it's the actual CircularROI object attached to the subset on the data that is wrong, and the translator is just referencing that object.

Address some comments from Jenneh review

Co-authored-by: Kyle Conroy <[email protected]>

to enable radial plot resizing

And clarify phot doc

pllim · 2022-02-08T22:16:32Z

@rosteen et al., I rebased and added documentation about the dither bug.

pllim · 2022-02-08T22:23:34Z

Doh, I even wrote it into the test... 😹

jdaviz/jdaviz/configs/imviz/tests/test_simple_aper_phot.py

Lines 66 to 68 in 9d35f7f

    
           # BUG: https://github.com/glue-viz/glue-astronomy/issues/52 
        
           # Sky should have been the same and the pix different, but not until bug is fixed. 
        
           # The aperture sum might be different too if mask is off limit in second image.

rosteen · 2022-02-09T13:54:00Z

jdaviz/app.py

+            tray_obj = self.widgets.get(tray_item['widget'].split('IPY_MODEL_')[1])
+            for bqplot_fig in tray_obj.bqplot_figs_resize:
+                bqplot_fig.layout.height = '99.9%'
+                bqplot_fig.layout.height = '100&'


Just checking - is this really supposed to be 100&, not 100%?

Hmm, @kecnry would know.

Good catch, yes it should be 100%

Okay, I pushed a commit to fix it. Makes me wonder what it was doing before. I didn't see anything weird in manual testing. Does this setting even do anything?

That's unclear to me 😆 . It also raises the question of whether there's a reason that one is 99.9% and the other is 100% (not that I didn't say a good reason, it could be an obtuse javascript thing).

@kecnry put that in when I noticed the radial plot won't resize when I resize the plugin tray.

It likely was defaulting to interpreting that as 100% anyways (I'm guessing/hoping?). This block is just forcing the layout to re-adjust to the changed size of the container (its kind of hacky by making a small change in the height the figure readjusts the width to fill the space - but its the same logic that already exists to handle viewer figure resizing).

rosteen

Approving, since the bug I noticed is documented and requires an upstream fix.

pllim · 2022-02-09T14:43:10Z

Thanks for the thorough reviews!

Please let me know which one is next so I can rebase that one.

pllim added this to the 2.3 milestone Jan 6, 2022

github-actions bot added documentation Explanation of code and concepts imviz labels Jan 6, 2022

pllim added the feature Feature request label Jan 6, 2022

pllim commented Jan 6, 2022

View reviewed changes

pllim mentioned this pull request Jan 6, 2022

add Imviz_RadialPlot_Example.ipynb #962

Closed

pllim force-pushed the radial-profiling branch from 08a20d9 to 1f185a0 Compare January 24, 2022 17:38

This comment has been minimized.

Sign in to view

maartenbreddels reviewed Jan 25, 2022

View reviewed changes

jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.py Outdated Show resolved Hide resolved

jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.py Outdated Show resolved Hide resolved

pllim force-pushed the radial-profiling branch from 1f185a0 to 6f277c0 Compare February 2, 2022 17:41

pllim marked this pull request as ready for review February 2, 2022 20:19

pllim requested review from duytnguyendtn, ibusko, javerbukh, kecnry, ojustino and rosteen as code owners February 2, 2022 20:19

pllim force-pushed the radial-profiling branch from 6c52ccb to ae64b7a Compare February 3, 2022 22:33

javerbukh approved these changes Feb 8, 2022

View reviewed changes

rosteen reviewed Feb 8, 2022

View reviewed changes

pllim and others added 8 commits February 8, 2022 17:09

FEAT: Radial profile in Imviz

3f1f8ef

Address some comments from Jenneh review

Radial profile: Replace mpl with bqplot

a258b7b

Co-authored-by: Kyle Conroy <[email protected]>

Change marker color to work in both light/dark modes

337ac47

resize all (registered) bqplot widgets in tray

faeb715

to enable radial plot resizing

tweak initial height and padding of radial plot

ba48f85

Simplify hub callback handler calls

b4f70e3

Clear plot before replotting.

c081f7a

And clarify phot doc

Document known bug that needs upstream fix

ea0a204

pllim force-pushed the radial-profiling branch from 1561be4 to ea0a204 Compare February 8, 2022 22:13

rosteen reviewed Feb 9, 2022

View reviewed changes

Fix layout height typo

081a720

rosteen approved these changes Feb 9, 2022

View reviewed changes

pllim merged commit ff9ef22 into spacetelescope:main Feb 9, 2022

pllim deleted the radial-profiling branch February 9, 2022 14:42

pllim mentioned this pull request Feb 21, 2022

Imviz: Add a different radial profile approach #1097

Merged

9 tasks

kecnry mentioned this pull request Feb 25, 2022

Radial profile plot by putting a plot in the plug-in tray #963

Closed

pllim mentioned this pull request Nov 14, 2022

No more centroiding in Simple Aperture Photometry plugin #1841

Merged

9 tasks

FEAT: Radial profile in Imviz simple aperture photometry plugin #1030

FEAT: Radial profile in Imviz simple aperture photometry plugin #1030

Conversation

pllim commented Jan 6, 2022 • edited Loading

Description

TODO

Screenshots (bqplot)

Checklist for package maintainer(s)

codecov bot commented Jan 6, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jenneh commented Jan 21, 2022

pllim commented Jan 21, 2022

kecnry commented Jan 24, 2022

pllim commented Jan 24, 2022

Jenneh commented Jan 24, 2022

pllim commented Jan 24, 2022

pllim commented Jan 24, 2022

This comment has been minimized.

javerbukh left a comment

Choose a reason for hiding this comment

kecnry commented Feb 8, 2022

pllim commented Feb 8, 2022

rosteen commented Feb 8, 2022 • edited Loading

pllim commented Feb 8, 2022

rosteen left a comment

Choose a reason for hiding this comment

pllim commented Feb 8, 2022 • edited Loading

rosteen commented Feb 8, 2022 • edited Loading

rosteen commented Feb 8, 2022

rosteen commented Feb 8, 2022

pllim commented Feb 8, 2022

pllim commented Feb 8, 2022

rosteen commented Feb 8, 2022

pllim commented Feb 8, 2022

pllim commented Feb 8, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosteen left a comment

Choose a reason for hiding this comment

pllim commented Feb 9, 2022

pllim commented Jan 6, 2022 •

edited

Loading

codecov bot commented Jan 6, 2022 •

edited

Loading

rosteen commented Feb 8, 2022 •

edited

Loading

pllim commented Feb 8, 2022 •

edited

Loading

rosteen commented Feb 8, 2022 •

edited

Loading