Debug Cubeviz batch photometry #3163

rosteen · 2024-08-22T14:25:29Z

Fixes a couple bugs with batch photometry in Cubeviz, one pre-existing and one to do with unit conversion. Opening as draft since I need to add a test or two still.

codecov · 2024-08-22T19:56:22Z

Codecov Report

Attention: Patch coverage is 88.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 88.44%. Comparing base (95d6008) to head (e0223c4).
Report is 44 commits behind head on main.

Files with missing lines	Patch %	Lines
...imviz/plugins/aper_phot_simple/aper_phot_simple.py	88.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3163      +/-   ##
==========================================
- Coverage   88.82%   88.44%   -0.39%     
==========================================
  Files         112      122      +10     
  Lines       17626    18276     +650     
==========================================
+ Hits        15657    16164     +507     
- Misses       1969     2112     +143

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

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

kecnry · 2024-08-23T11:45:01Z

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

+        if isinstance(data, list):
+            data = data[0]


don't love this, but this whole method will probably go away with #3156, and I can't think of a better way to do it with the current setup.

cc @cshanahan1

this will go away at some point (im trying to consolidate everything that listens to unit changes to one method in that PR), but it also seems like its necessary here for now

CHANGES.rst

pllim · 2024-08-26T15:47:18Z

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

+                    self.is_cube = True
+                    break
+                else:
+                    self.is_cube = False


This can be set outside of for? (For False)

I personally prefer this to setting it to False first and then overriding after 🤷‍♂️

In that case, self.is_cube = False runs every iteration (until loop ends or set to True) whether that assignment is need or not. But given all the other performance bottlenecks we have, this is probably nothing, so okay for me if you prefer this.

I have no preference, it could be set first since it's breaking once set to True.

oh I misunderstood, yes, the else could/should be indented out one level to be inline with the for

pllim · 2024-08-26T15:48:40Z

jdaviz/configs/imviz/tests/test_simple_aper_phot.py

+    uc_plugin = cubeviz_helper.plugins['Unit Conversion']
+
+    fv = cubeviz_helper.app.get_viewer('flux-viewer')
+    fv.apply_roi(CircularROI(xc=5, yc=5, radius=2))


Why not use the load region API?

I'm running into test failures after switching to load_regions, trying to figure out if they're real or I'm calling something incorrectly.

pllim · 2024-08-26T15:49:18Z

jdaviz/configs/imviz/tests/test_simple_aper_phot.py

+
+    phot_plugin.calculate_batch_photometry()
+
+    assert len(phot_plugin.table) == 2


Should we also at least check that the sums make sense?

I was lazily assuming that the existing tests already covered that, and we just needed to make sure that the existing code ran multiple times here. I could add more checks though.

Lines are probably covered but I am unsure about how many use cases we're actually covering here and there. My take is that more tests do not hurt but it is up to you whether you want to add more checks or not.

cshanahan1

tested this and it works well, im able to use batch mode and the results are repeatable through unit conversions

cshanahan1 · 2024-08-26T15:54:31Z

jdaviz/configs/imviz/tests/test_simple_aper_phot.py

+
+    phot_plugin.calculate_batch_photometry()
+
+    assert len(phot_plugin.table) == 2


maybe you could follow some of the other tests here and compare the output tables between unit conversions, to make sure the results are the same?

cshanahan1 · 2024-08-26T15:57:07Z

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

+        if isinstance(data, list):
+            data = data[0]


this will go away at some point (im trying to consolidate everything that listens to unit changes to one method in that PR), but it also seems like its necessary here for now

rosteen · 2024-08-26T20:53:06Z

@cshanahan1 @pllim I think I addressed all of your comments that needed changes.

pllim

LGTM except maybe a rogue raise leftover from debugging.

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

pllim · 2024-08-26T21:46:30Z

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

Changes in this file are mainly changing things that @cshanahan1 added earlier, so I would prefer she review as well.

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

pllim · 2024-08-27T13:53:54Z

Isn't unit conversion stuff new in v4? Can/should this really be backported?

rosteen · 2024-08-27T14:14:40Z

Isn't unit conversion stuff new in v4? Can/should this really be backported?

Hm, there was nothing incompatible with 3.10.x until I had to add the display_unit checks in the last commit. Might have to do a manual backport of the things unrelated to unit conversion.

* Debugging Cubeviz batch photometry * Fix changing units when in batch mode * Working on adding cubeviz batch photometry test * Add simple test * Codestyle * Add comment about future changes * Move changelog to 3.10.4 * Don't convert if display unit hasn't been initialized * Update test * Codestyle * Update jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.py Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com> --------- Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com> (cherry picked from commit 19a8f56)

pllim · 2024-09-05T14:48:07Z

Backport failed and abandoned. I updated the milestone here. Can you please open follow-up PR to move the change log? Thanks!

* Debugging Cubeviz batch photometry * Fix changing units when in batch mode * Working on adding cubeviz batch photometry test * Add simple test * Codestyle * Add comment about future changes * Move changelog to 3.10.4 * Don't convert if display unit hasn't been initialized * Update test * Codestyle * Update jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.py Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com> --------- Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

rosteen added 2 commits August 20, 2024 11:43

Debugging Cubeviz batch photometry

1c2baab

Fix changing units when in batch mode

547d831

rosteen added bug Something isn't working cubeviz labels Aug 22, 2024

rosteen added this to the 4.0 milestone Aug 22, 2024

github-actions bot added imviz plugin Label for plugins common to multiple configurations labels Aug 22, 2024

rosteen added 3 commits August 22, 2024 11:47

Working on adding cubeviz batch photometry test

37087c4

Add simple test

b847c24

Codestyle

bad39c0

rosteen marked this pull request as ready for review August 22, 2024 19:31

rosteen requested review from javerbukh, pllim, kecnry, bmorris3, haticekaratay, cshanahan1 and gibsongreen as code owners August 22, 2024 19:31

kecnry approved these changes Aug 23, 2024

View reviewed changes

kecnry reviewed Aug 23, 2024

View reviewed changes

CHANGES.rst Outdated Show resolved Hide resolved

Add comment about future changes

c8fe010

kecnry added the Ready for final review label Aug 23, 2024

Move changelog to 3.10.4

b641728

rosteen modified the milestones: 4.0, 3.10.4 Aug 23, 2024

rosteen added the backport-v3.10.x on-merge: backport to v3.10.x label Aug 23, 2024

pllim reviewed Aug 26, 2024

View reviewed changes

cshanahan1 reviewed Aug 26, 2024

View reviewed changes

rosteen added 3 commits August 26, 2024 15:51

Don't convert if display unit hasn't been initialized

c8f07ef

Update test

4556ff7

Codestyle

9328fdb

pllim reviewed Aug 26, 2024

View reviewed changes

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

e0223c4

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

pllim approved these changes Aug 27, 2024

View reviewed changes

rosteen merged commit 19a8f56 into spacetelescope:main Sep 5, 2024
17 of 19 checks passed

This comment was marked as resolved.

Sign in to view

lumberbot-app bot added the Still Needs Manual Backport label Sep 5, 2024

rosteen mentioned this pull request Sep 5, 2024

Debug Cubeviz batch photometry (backport #3163 onto v3.10.x) #3179

Closed

pllim removed the Still Needs Manual Backport label Sep 5, 2024

pllim modified the milestones: 3.10.4, 4.0 Sep 5, 2024

pllim removed the backport-v3.10.x on-merge: backport to v3.10.x label Sep 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Debug Cubeviz batch photometry #3163

Debug Cubeviz batch photometry #3163

rosteen commented Aug 22, 2024

codecov bot commented Aug 22, 2024 •

edited

Loading

kecnry Aug 23, 2024

kecnry Aug 23, 2024

cshanahan1 Aug 26, 2024

pllim Aug 26, 2024 •

edited

Loading

kecnry Aug 26, 2024

pllim Aug 26, 2024

rosteen Aug 26, 2024

kecnry Aug 26, 2024 •

edited

Loading

pllim Aug 26, 2024

rosteen Aug 26, 2024

pllim Aug 26, 2024

rosteen Aug 26, 2024 •

edited

Loading

pllim Aug 26, 2024

cshanahan1 left a comment

cshanahan1 Aug 26, 2024

cshanahan1 Aug 26, 2024

rosteen commented Aug 26, 2024

pllim left a comment

pllim Aug 26, 2024

pllim commented Aug 27, 2024

rosteen commented Aug 27, 2024

This comment was marked as resolved.

pllim commented Sep 5, 2024


		phot_plugin.calculate_batch_photometry()

		assert len(phot_plugin.table) == 2

Debug Cubeviz batch photometry #3163

Debug Cubeviz batch photometry #3163

Conversation

rosteen commented Aug 22, 2024

codecov bot commented Aug 22, 2024 • 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

pllim Aug 26, 2024 • edited Loading

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 Aug 26, 2024 • edited Loading

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 Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cshanahan1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosteen commented Aug 26, 2024

pllim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim commented Aug 27, 2024

rosteen commented Aug 27, 2024

This comment was marked as resolved.

pllim commented Sep 5, 2024

codecov bot commented Aug 22, 2024 •

edited

Loading

pllim Aug 26, 2024 •

edited

Loading

kecnry Aug 26, 2024 •

edited

Loading

rosteen Aug 26, 2024 •

edited

Loading