Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Debug Cubeviz batch photometry #3163

Merged
merged 11 commits into from
Sep 5, 2024

Conversation

rosteen
Copy link
Collaborator

@rosteen rosteen commented Aug 22, 2024

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.

@rosteen rosteen added bug Something isn't working cubeviz labels Aug 22, 2024
@rosteen rosteen added this to the 4.0 milestone Aug 22, 2024
@github-actions github-actions bot added imviz plugin Label for plugins common to multiple configurations labels Aug 22, 2024
@rosteen rosteen marked this pull request as ready for review August 22, 2024 19:31
Copy link

codecov bot commented Aug 22, 2024

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 151 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.

Comment on lines +219 to +220
if isinstance(data, list):
data = data[0]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
@rosteen rosteen modified the milestones: 4.0, 3.10.4 Aug 23, 2024
@rosteen rosteen added the 💤backport-v3.10.x on-merge: backport to v3.10.x label Aug 23, 2024
self.is_cube = True
break
else:
self.is_cube = False
Copy link
Contributor

@pllim pllim Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@kecnry kecnry Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use the load region API?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


phot_plugin.calculate_batch_photometry()

assert len(phot_plugin.table) == 2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@rosteen rosteen Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@cshanahan1 cshanahan1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


phot_plugin.calculate_batch_photometry()

assert len(phot_plugin.table) == 2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment on lines +219 to +220
if isinstance(data, list):
data = data[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator Author

rosteen commented Aug 26, 2024

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

Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except maybe a rogue raise leftover from debugging.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

pllim commented Aug 27, 2024

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

@rosteen
Copy link
Collaborator Author

rosteen commented Aug 27, 2024

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.

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

This comment was marked as resolved.

rosteen added a commit to rosteen/jdaviz that referenced this pull request Sep 5, 2024
* 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 pllim modified the milestones: 3.10.4, 4.0 Sep 5, 2024
@pllim pllim removed the 💤backport-v3.10.x on-merge: backport to v3.10.x label Sep 5, 2024
@pllim
Copy link
Contributor

pllim commented Sep 5, 2024

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

cshanahan1 pushed a commit to cshanahan1/jdaviz that referenced this pull request Sep 9, 2024
* 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 a commit to rosteen/jdaviz that referenced this pull request Oct 29, 2024
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cubeviz imviz plugin Label for plugins common to multiple configurations Ready for final review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants