Fix aperture photometry result for WCS linked dithered non-reference data #1524

pllim · 2022-07-24T16:48:54Z

Description

This pull request is to address a critical but subtle bug where aperture photometry gives inaccurate result when photometry is performed on a non-reference data that is dithered from reference data and they are linked by WCS.

This works around glue-viz/glue-astronomy#52

Fixes #1508

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

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

pllim · 2022-08-24T02:39:23Z

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

+
+    # Sometimes the mask is smaller than radial_r
+    if radial_cutout.shape != reg_bb.shape:
+        radial_r = radial_r[:radial_cutout.shape[0], :radial_cutout.shape[1]]


@larrybradley , in my test case (10 x 10 images dithered by 1 pix with mask off-limit a little for second image when linked by WCS), ApertureStat bounding box shape can be different from the cutout shape. Is this the right way to handle it?

codecov · 2022-08-24T02:58:05Z

Codecov Report

Base: 86.35% // Head: 86.37% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (6cfe418) compared to base (1c9fef2).
Patch coverage: 95.83% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1524      +/-   ##
==========================================
+ Coverage   86.35%   86.37%   +0.01%     
==========================================
  Files          94       94              
  Lines        9410     9428      +18     
==========================================
+ Hits         8126     8143      +17     
- Misses       1284     1285       +1

Impacted Files	Coverage Δ
...imviz/plugins/aper_phot_simple/aper_phot_simple.py	`92.05% <95.65%> (+0.12%)`	⬆️
jdaviz/configs/imviz/helper.py	`96.61% <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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

kecnry

Does anything (cached background, populated inputs based on the subsets, etc) in the aperture photometry plugin need to listen to changes in the linking and update?

Besides that question, it's hard to test and know what changes I should expect to see, but I couldn't get it to crash and the code looks reasonable! 🤷

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

pllim · 2022-08-25T21:56:38Z

aperture photometry plugin need to listen

I do not think so because the plugin grabs stuff out of data only when Calculate is pressed. So if linking has changed, etc, that would naturally propagate...

Theoretically, we can test that by expanding the unit test to relink back to pixels and check if the answer will be wrong again. Do you want me to do that?

kecnry · 2022-08-26T12:09:10Z

I do not think so because the plugin grabs stuff out of data only when Calculate is pressed.

It looks like background_value is used by vue_do_aper_phot which defaults when the background subset is changed. I think a change to linking might just need to trigger _on_subset_update for all subsets (since the pixels the subset covers may have been affected by a change in the linking).

I wonder if there's any way we could do this from the linking plugin instead - by sending subset update events to anything that already listens to them, since this likely applies to more places than just aperture photometry?

pllim · 2022-08-27T00:44:37Z

Re: #1524 (comment)

Okay, I see what you are saying. The background does not auto-update when linking is changed but it does update when you re-select the Subset.

Since you are working on a new "link message" over at #1598, maybe I can add a note about this behavior and revisit this later.

pllim · 2022-08-27T00:54:16Z

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

+                    if Version(astropy.__version__) <= Version('5.1'):
+                        fitter_kw = {}
+                    else:
+                        fitter_kw = {'filter_non_finite': True}


The coverage will complain about this because this is tested in the dev deps job but we do not calculate coverage for dev deps.

kecnry · 2022-08-29T13:00:42Z

Since you are working on a new "link message" over at #1598, maybe I can add a note about this behavior and revisit this later.

Looking through the plugins, I think photometry might be the only place affected (unless maybe subset options is as well?). But you're right, if we get #1598 in first (which is ready for review), then I think it should be as easy as listening for that event and treating all selected subsets (aperture and background) as needing updating. I think it would be nice to get it in here so that we don't need those blurbs in the docs - or we should consider disabling parts of plugins accordingly if we're not going to get it fixed soon.

pllim · 2022-08-30T21:58:01Z

@kecnry , I have updated this PR to use #1598 but I did have to move when the message is emitted to the end for this to work.

kecnry · 2022-08-31T12:06:49Z

jdaviz/configs/imviz/helper.py

+        # Only broadcast after success.
+        app.hub.broadcast(LinkUpdatedMessage(link_type,
+                                             wcs_fallback_scheme == 'pixels',
+                                             wcs_use_affine,
+                                             sender=app))


this move should be ok since the spinner state is updated independently of the event, the only difference will be that when calling linking from the API the spinner will now start, the linking will happen, THEN the radio buttons will update and the spinner will clear (whereas before the radio buttons would switch before the linking actually started). I can see how for other uses of checking the linking (like in this PR) it is more beneficial to receive the message after the linking has been completed.

kecnry · 2022-08-31T12:08:10Z

docs/imviz/plugins.rst

+For the best experience, it is recommended that you decide what kind of
+link you want and set it at the beginning of your Imviz session,
+rather than later.
+


can this be removed now?

I want to keep it. I think it is a good recommendation regardless.

kecnry · 2022-08-31T12:11:49Z

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

+        # Force background auto-calculation to update when linking has changed.
+        self._bg_subset_selected_changed()


Does self._subset_selected_changed() need to be called (or since it's only caching annulus information, is that "immune" to changes in linking)?

Good point. I didn't test the annulus case. I'll do it when I get back, or feel free to push commits here while I am gone if this is in a rush.

If we end up needing _subset_selected_changed(), then I think we don't have to call _bg_subset_selected_changed() because the former also triggers the latter.

I pushed a commit to also fix the annulus case. Please re-review. Good catch. Thanks!

done on non-reference image if images are linked by WCS.

Handle case where aperstat bbox different shape from cutout. Properly cast radii as int for binning.

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

if linking is changed in the middle of a aper phot session

kecnry

LGTM (assuming CI still passes and assuming changes to tests are correct - I didn't double check the changed numbers)!

pllim · 2022-09-07T14:24:04Z

changed numbers

I think the test in the diff behaves as expected. It now reports consistent sky coordinates for dithered data linked by WCS, but different pixel coordinates and sums; the latter because when the aperture mask takes account of the WCS linking, in the second dataset, it is a little off the image. Does this make sense?

camipacifici

Fixes the problem on my side too.

pllim · 2022-09-08T17:58:52Z

Thanks for the thorough reviews!

pllim added the bug Something isn't working label Jul 24, 2022

pllim added this to the 2.9 milestone Jul 24, 2022

github-actions bot added the imviz label Jul 24, 2022

pllim force-pushed the aperphot-wcs-dithered branch from 33a8b7e to e5d71b3 Compare August 23, 2022 22:54

pllim mentioned this pull request Aug 23, 2022

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

Open

pllim commented Aug 24, 2022

View reviewed changes

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

pllim commented Aug 24, 2022

View reviewed changes

pllim marked this pull request as ready for review August 24, 2022 03:09

pllim requested review from duytnguyendtn, rosteen, javerbukh, ojustino and kecnry as code owners August 24, 2022 03:09

rosteen modified the milestones: 2.9, 2.10 Aug 24, 2022

pllim force-pushed the aperphot-wcs-dithered branch from 7381626 to 31c4d6c Compare August 24, 2022 15:50

kecnry reviewed Aug 25, 2022

View reviewed changes

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

This comment was marked as resolved.

Sign in to view

pllim modified the milestones: 2.10, 2.11 Aug 26, 2022

pllim force-pushed the aperphot-wcs-dithered branch from 85f9009 to a507933 Compare August 27, 2022 00:52

pllim commented Aug 27, 2022

View reviewed changes

pllim force-pushed the aperphot-wcs-dithered branch from a507933 to 438c922 Compare August 30, 2022 21:56

kecnry reviewed Aug 31, 2022

View reviewed changes

pllim and others added 7 commits September 7, 2022 09:33

BUG: Fix inaccurate photometry when it is

d7c087a

done on non-reference image if images are linked by WCS.

Allow filter_non_finite for astropy 5.2 fitter.

8e53e89

Handle case where aperstat bbox different shape from cutout. Properly cast radii as int for binning.

Fix test

98d20aa

Use simpler app access

12adc70

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

DOC: Clarify behavior expectation

b7c85ca

if linking is changed in the middle of a aper phot session

Fix bg auto calc on link update

4b75709

Also auto update annulus calc on relinking

6cfe418

pllim force-pushed the aperphot-wcs-dithered branch from 438c922 to 6cfe418 Compare September 7, 2022 14:15

kecnry approved these changes Sep 7, 2022

View reviewed changes

kecnry added the Ready for final review label Sep 7, 2022

camipacifici mentioned this pull request Sep 8, 2022

Imviz: Clarify how subset is extracted when multiple data are loaded (was: explicit layer in subset plugin) #1629

Open

camipacifici self-requested a review September 8, 2022 15:21

camipacifici approved these changes Sep 8, 2022

View reviewed changes

pllim merged commit e76482f into spacetelescope:main Sep 8, 2022

pllim deleted the aperphot-wcs-dithered branch September 8, 2022 17:58

pllim mentioned this pull request Nov 14, 2022

No more centroiding in Simple Aperture Photometry plugin #1841

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix aperture photometry result for WCS linked dithered non-reference data #1524

Fix aperture photometry result for WCS linked dithered non-reference data #1524

pllim commented Jul 24, 2022 •

edited

Loading

pllim Aug 24, 2022

codecov bot commented Aug 24, 2022 •

edited

Loading

kecnry left a comment

pllim commented Aug 25, 2022

kecnry commented Aug 26, 2022

This comment was marked as resolved.

pllim commented Aug 27, 2022

pllim Aug 27, 2022

kecnry commented Aug 29, 2022

pllim commented Aug 30, 2022

kecnry Aug 31, 2022

kecnry Aug 31, 2022

pllim Aug 31, 2022

kecnry Aug 31, 2022

pllim Aug 31, 2022

pllim Sep 7, 2022

kecnry left a comment

pllim commented Sep 7, 2022

camipacifici left a comment

pllim commented Sep 8, 2022

		# Force background auto-calculation to update when linking has changed.
		self._bg_subset_selected_changed()

Fix aperture photometry result for WCS linked dithered non-reference data #1524

Fix aperture photometry result for WCS linked dithered non-reference data #1524

Conversation

pllim commented Jul 24, 2022 • edited Loading

Description

Checklist for package maintainer(s)

Choose a reason for hiding this comment

codecov bot commented Aug 24, 2022 • edited Loading

Codecov Report

kecnry left a comment

Choose a reason for hiding this comment

pllim commented Aug 25, 2022

kecnry commented Aug 26, 2022

This comment was marked as resolved.

pllim commented Aug 27, 2022

Choose a reason for hiding this comment

kecnry commented Aug 29, 2022

pllim commented Aug 30, 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

kecnry left a comment

Choose a reason for hiding this comment

pllim commented Sep 7, 2022

camipacifici left a comment

Choose a reason for hiding this comment

pllim commented Sep 8, 2022

pllim commented Jul 24, 2022 •

edited

Loading

codecov bot commented Aug 24, 2022 •

edited

Loading