spacetelescope / jdaviz

JWST astronomical data analysis tools in the Jupyter platform
https://jdaviz.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
136 stars 73 forks source link

Imviz: Maybe we should not use Subset at all? #2238

Open pllim opened 1 year ago

pllim commented 1 year ago

glue Subset is meant to link area of one data to another, e.g., you select some magnitudes from a H-R diagram and then it highlights the stars on an image. However, in Imviz, we are using Subset basically as apertures, e.g., you draw a circle around the star and you want the flux in the circle. When using Subset as an aperture and you have multiple data loaded and they are linked by WCS, we have to put in extra logic to ensure that the aperture is correctly moved in a data that isn't the reference data. As a result, @astrofrog had posed the question of whether Imviz should really be using Subset at all.

By removing Subset from Imviz, we would be able to simplify a lot of the code. Perhaps we really should be using markers. Alas, there is currently no drop-in replacement for the way we handle Subset in markers. In addition, all the work we did to improve spatial Subset for Imviz (e.g., annulus) would be wasted. Additionally, any support for MaskedSubset will also go away (bye bye, Space Invader). The immediate upside is that we no longer have to worry about performance issues when drawing too many Subsets, and there will also be no difference in workflow between drawing one aperture vs. marking 1000 stars.

Some work that might be involved:

See Also

🐱 🐱

camipacifici commented 1 year ago

What about subsets in the other vizs? Would spectral subsets be replaced as well? How about spatial/spectral subsets in Cubeviz?

pllim commented 1 year ago

What about subsets in the other vizs?

🥫 🐛 💥

Would spectral subsets be replaced as well?

I don't think so. Markers idiom don't really work in spectral regime, at least not the way they were designed in astrowidgets.

How about spatial/spectral subsets in Cubeviz?

Would anyone try to do aperture photometry in Cubeviz? I would say Subset is actually useful in Cubeviz because you select it to collapse a spectrum, which is what it is supposedly designed to do. That is why in the post above, I mentioned, "decoupling Cubeviz and Mosviz..."

pllim commented 1 year ago

Crazy idea: What if we retain the Subset draw tools but when you finalize the drawing, it creates a data point in the scatter plot instead of a subset? The big drawback is that I am not sure how to get bqplot to support new shapes in its scatter plot options. Current options are limited to this:

https://github.com/bqplot/bqplot/blob/ff9848d566cb479cb6800fb8901bf91fcc6c7939/bqplot/marks.py#L620-L621

The documentation is horrible, so I cannot tell if you can rotate the rectangle marker or not; probably not. And we'll need some custom workaround to support circular annulus drawing since you have to represent one data point with 2 circles, and remember that it is an annulus and not just 2 random circles.

kecnry commented 1 year ago

I do not think that scatter markers are a feasible path forward (they are represented by a point, whereas we need an arbitrary shape that is aware of the zoom-level, etc). If it turns out subsets as they exist are not appropriate, we might want to investigate the ability to disable some aspects of the subset logic while keeping the rest of the internals - we definitely want data masking/retrieval across different linked data-layers, but since we know all of those layers will be images with identity or WCS links, we might be able to have extra assumptions which circumvent the issues.

pllim commented 1 year ago

If we want image viewer + scatter plot + histogram viewers all talking to one another in Imviz, we cannot get rid of Subset completely, so might have to reduce the scope of this work just to Simple Aperture Photometry plugin.