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

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

Open
pllim opened this issue Jun 5, 2023 · 5 comments
Open

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

pllim opened this issue Jun 5, 2023 · 5 comments

Comments

@pllim
Copy link
Contributor

pllim commented Jun 5, 2023

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:

  • Refactor all the markers API to not link markers to table. Instead draw them on-demand using Scatter in bqplot (this is what Matt Craig did in his POC of bqplot backend in astrowidgets).
  • Add support of drawing different shapes with markers.
  • Add support for moving and editing markers.
  • Remove all Subset logic from Imviz. This might also involve decoupling Cubeviz and Mosviz image viewers from Imviz as well (not sure).
  • Tell all the crying users that everything will be okay... eventually. They will have to fix their notebooks and change to a new workflow.
  • Update all the Imviz videos out there.

See Also

🐱 🐱

@camipacifici
Copy link
Contributor

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

@pllim
Copy link
Contributor Author

pllim commented Jun 5, 2023

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

pllim commented Jun 7, 2023

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

kecnry commented Jun 7, 2023

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

pllim commented Sep 13, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants