spacetelescope / jdaviz

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

[BUG] Imviz: contrast/bias display gives crazy values when you zoom in #774

Closed eteq closed 3 years ago

eteq commented 3 years ago

I expect the contrast/bias adjust to behve roughly like ds9: regardless of zoom level, dragging left->right should adjust the bias from the faintest to the brightest pixel, and bottom to top should adjust the contrast over a similarly "reasonable" range.

Right now, it behaves fairly well at the default zoom. But if I zoom in or out I start getting crazy values - like:

>>> viewer.state.layers[0].bias, viewer.state.layers[0].contrast
(-6.5193250089596155, 154928091175.2888)

which is nonesense because bias should by on [0,1] and contrast should be on some sane range (maybe [0,4] ? that's what the slider bar gives, although I don't understand that number exactly like I do for bias).

@pllim pointed me to https://github.com/spacetelescope/jdaviz/blob/db81d33e4debe2a967679533affdd1ad46485c25/jdaviz/configs/imviz/plugins/tools.py#L128 which seems to be where this is derived. Not sure I understand it, but that seems like a place to start...

cc @pllim @astrofrog

🐱

pllim commented 3 years ago

As per offline conversation between Erik and I, the max for bias is supposed to be 1 and contrast 4 according to https://github.com/glue-viz/glue-jupyter/blob/8e3bb27fda607980317978026fc7a51d0aeb5afa/glue_jupyter/common/state_widgets/layer_image.vue#L60-L67 .

However back in #553 (xref #516) , @astrofrog pointed me to a formula in https://github.com/glue-viz/glue/blob/6b968b352bc5ad68b95ad5e3bb25550782a69ee8/glue/viewers/image/qt/contrast_mouse_mode.py#L34-L36 .

I think what we really need is the cursor X/Y positions w.r.t. the display window, not the data. Maybe @astrofrog or @maartenbreddels can tell me how to grab this from the glue/bqplot backend.

pllim commented 3 years ago

If #775 looks fine, maybe we don't need SME inputs after all.