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

convert flux cubes to per-square-pixel surface brightness cubes #3156

Merged
merged 77 commits into from
Sep 20, 2024

Conversation

cshanahan1
Copy link
Contributor

@cshanahan1 cshanahan1 commented Aug 19, 2024

This PR implements support of per-pixel surface brightness cubes. Cubes loaded in flux units will be loaded as flux per square pixel surface brightness cubes. Cubes loaded directly in flux/pix2 are now supported as well.

This also fixes an issue with units in moment maps that was required for this work (may overlap some with #3116)

Another issue with batch mode photometry not showing units in the plugin is fixed by this as well

Also adds some improvement of support for cubes in counts

@kecnry
Copy link
Member

kecnry commented Aug 30, 2024

Loading a flux cube, setting the flux or sb switch in unit conversion to surface brightness, and then changing flux unit raises a bunch of errors, likely similar situations as #3139.

@cshanahan1
Copy link
Contributor Author

That seems to be working fine for me, interesting. I'll try to reproduce the errors

@kecnry
Copy link
Member

kecnry commented Aug 30, 2024

I was using:

from jdaviz.conftest import _create_spectrum1d_cube_with_fluxunit
data = _create_spectrum1d_cube_with_fluxunit(fluxunit=u.Jy, shape=(2,2,4), with_uncerts=True)
cubeviz.load_data(data)

@rosteen
Copy link
Collaborator

rosteen commented Sep 4, 2024

Would you rebase to main and fix the conflicts, please?

@cshanahan1
Copy link
Contributor Author

I was using:

from jdaviz.conftest import _create_spectrum1d_cube_with_fluxunit
data = _create_spectrum1d_cube_with_fluxunit(fluxunit=u.Jy, shape=(2,2,4), with_uncerts=True)
cubeviz.load_data(data)

This doesn't give me any errors after loading that cube:
uc = cubeviz_helper.plugins['Unit Conversion']._obj
setting to SB
uc.flux_or_sb.selected = 'Surface Brightness'
and changing flux unit
uc.flux_unit.selected = 'Jy'

it also works in the UI. Was there some other way to trigger this error?

@kecnry
Copy link
Member

kecnry commented Sep 5, 2024

here's a full example to reproduce with the current branch.

from jdaviz import Cubeviz
from jdaviz.conftest import _create_spectrum1d_cube_with_fluxunit
from astropy import units as u
data = _create_spectrum1d_cube_with_fluxunit(fluxunit=u.Jy, shape=(2,2,4), with_uncerts=True)
cubeviz = Cubeviz()
cubeviz.load_data(data)
cubeviz.show()
uc = cubeviz.plugins['Unit Conversion']
uc.open_in_tray()
uc.flux_or_sb = 'Surface Brightness' # at this point mouseover in the spectrum viewer is broken, but that can probably be a follow-up ticket
uc.flux_unit = 'MJy'
uc.flux_or_sb = 'Flux'

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.

Partial review. Will continue later.

Note to self: test_cubeviz_aperphot_cube_sr_and_pix2

jdaviz/configs/cubeviz/plugins/parsers.py Outdated Show resolved Hide resolved
jdaviz/configs/cubeviz/plugins/parsers.py Outdated Show resolved Hide resolved
jdaviz/configs/cubeviz/plugins/parsers.py Outdated Show resolved Hide resolved
jdaviz/configs/cubeviz/plugins/parsers.py Outdated Show resolved Hide resolved
jdaviz/configs/cubeviz/plugins/parsers.py Outdated Show resolved Hide resolved
jdaviz/configs/cubeviz/plugins/parsers.py Show resolved Hide resolved
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.

I think I will stop here. Looking like you are pushing commits while I am reviewing. I don't want to end up reviewing outdated content.

rosteen added a commit that referenced this pull request Sep 20, 2024
* Fix NaN handling in cube fitting and initial fixes for unit conversion/model fitting interaction

* Remove debugging prints, add comment for context

* Codestyle, changelog

* Reestimate parameters when cube fitting is toggled

* Only reestimate if spectral y type isn't SB

* Codestyle

* Changelog

* Fix initializing linear component for cube fit

* Handle linear component estimation for cube case

* Only reshape here in 3D case

* Skip tests that need #3156

* Respect selected display units when initializing model components

* Use app._get_display_unit instead of relying on Unit Conversion

* Add equivalency here

* Only reestimate here if sb unit != spectral_y unit

* Don't automatically reestimate when toggling cube fit, make the user do it

* Remove print

* Check for warning in test after cube toggle

Fix test

Codestyle

* Add test for cube fitting after flux unit change

* Add a to-do about a test for unit conversion with equivalency

* Fix failing test

* Back to parallel processing post-debugging
Copy link
Collaborator

@rosteen rosteen left a comment

Choose a reason for hiding this comment

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

Skipped tests that needed #3190 + this are unskipped and passing, approved. Thanks for all the work on this!

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.

I added 2 follow up items to #3192 . My one comment about the import is not that important since CI is passing. So approving.

Feel free to merge if you don't want to address that comment or after you address it.

Thanks!

@cshanahan1 cshanahan1 enabled auto-merge (squash) September 20, 2024 18:25
@cshanahan1 cshanahan1 merged commit 7391556 into spacetelescope:main Sep 20, 2024
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cubeviz plugin Label for plugins common to multiple configurations specviz
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants