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

Respect loaded mask cube properly in spectral extraction. #3319

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

rosteen
Copy link
Collaborator

@rosteen rosteen commented Nov 25, 2024

Fixes #3312.

Close #2718

@rosteen rosteen added bug Something isn't working cubeviz backport-v4.0.x on-merge: backport to v4.0.x labels Nov 25, 2024
@rosteen rosteen added this to the 4.0.1 milestone Nov 25, 2024
@github-actions github-actions bot added specviz plugin Label for plugins common to multiple configurations labels Nov 25, 2024
@rosteen rosteen force-pushed the extract-cube-masks-fix branch 2 times, most recently from 209915f to bab8070 Compare November 25, 2024 21:23
@@ -22,6 +22,7 @@ class Cubeviz(CubeConfigHelper, LineListMixin):

_loaded_flux_cube = None
_loaded_uncert_cube = None
_loaded_mask_cube = None
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe give @javerbukh co-author credit given #2718 ?

Copy link
Member

@kecnry kecnry left a comment

Choose a reason for hiding this comment

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

The code itself looks good to me - just a few workflow related questions to consider:

Do we want (now or as a a follow-up ticket) to have a dropdown to select the mask cube? Should the default be to apply the mask or to not apply it?

What happens if the intersection between an aperture and the mask results in no pixels?

Should the mask be used in other plugins as well (aperture photometry, moment map, collapse, etc)?

@camipacifici
Copy link
Contributor

I think that if it is used, it should be used everywhere (it can be a follow up issue). Can there maybe be an option at load_data to load AND use the mask?

I would not set it to use by default, but maybe there could be a warning at loading like "there is a mask extension, if you want to load it do blah"

@camipacifici
Copy link
Contributor

Visualize the mask cube would be very useful!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v4.0.x on-merge: backport to v4.0.x bug Something isn't working cubeviz plugin Label for plugins common to multiple configurations specviz
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Masks not handled properly in cube spectral extraction
4 participants