search

glue-viz / glue

Linked Data Visualizations Across Multiple Files
http://glueviz.org
Other
721 stars 152 forks source link

Allow setting slices or `subset_state` for determining min/max levels in `StateAttributeLimitsHelper` #2476

Closed dhomeier closed 2 months ago

dhomeier commented 5 months ago

Description

This enables passing a subset_state to the compute_statistic calls in StateAttributeLimitsHelper to derive limits based on that subset only and adds a set_slice convenience method to create a SliceSubsetState for that purpose. That function is also exposed through ImageLayerState.

TBD: StateAttributeLimitsHelper should automatically update (re-run statistics) on changes to subset_state; currently have to trigger this via setting percentile or other attributes (see tests). Adding this to Callbacks looks a bit hackish, but update seems to work so far.

Perhaps should test multi-dimensional case as well and explore other types of subsets (SliceSubsetState has special handling in compute_statistic).

dhomeier commented 2 months ago

@astrofrog I've minimised the callback interaction now while percentile='Custom', hope that will not overly impact performance. Also tried to address a case where calling update_values with force=True would raise an exception in https://github.com/glue-viz/glue/blob/9ea36363539229c384ebc06f2445c5d4eceec321/glue/core/state_objects.py#L326 – don't know if there is any sensible way to deal with 'Custom' percentiles for that case otherwise. I thought about alternatively removing the callback entirely while percentile is set to 'Custom', but could not figure out what the actual callback function is percentile is calling, or how.

@kecnry is this implementation, with making ImageLayerState.global_limits = False settable programmatically, complete on the glue side for your needs? Some of the pulldowns in Cubeviz, like Stretch Percentile, are already defined here, other's aren't, so I don't see a clear correspondence. Also I am not sure how the callback function is to be set up for a DeferredDrawSelectionCallbackProperty, if I tried to set this up analogously to percentile.

dhomeier commented 2 months ago

@astrofrog is there anything else needed here, if the 'Custom' handling is acceptable? There are no tests yet, but I did not find any existing tests directly on attribute_lim_helper, are the limits tested anywhere indirectly?

astrofrog commented 2 months ago

@dhomeier - I'll put this at the top of my list to look at for tomorrow morning.

kecnry commented 2 months ago

I'm planning to test this new implementation within jdaviz in the next week or so and will report back here

kecnry commented 2 months ago

This seems to work well for what we need in jdaviz (see https://github.com/spacetelescope/jdaviz/pull/2814), thanks! I'm slightly worried the state name global_limits could cause confusion that it somehow is related to x/y limits, rather than the vmin/vmax used for stretch. In that jdaviz PR, I refer to this switch as stretch_global in the intermediate layer between the user and glue-state, although I'm not sure that would be general enough for the internal glue name.

dhomeier commented 2 months ago

Yes, the naming is definitely still up to suggestions. I did not use anything related to stretch as that is not used anywhere within the relevant glue code – it is basically controlling the behaviour of an AttributeLimitsHelper, thus my choice. But it is also a property of a StretchStateMixin subclass – maybe global_stretch_limits would make most sense, if it wasn't so long. ;-)

astrofrog commented 2 months ago

@dhomeier - this looks good! I have a couple of small suggestions but I need to play around with this tomorrow first to see if those make sense.

dhomeier commented 2 months ago

Looking at the description of StretchStateMixin, stretch_global actually seems appropriate enough.

dhomeier commented 2 months ago

OK, just rebased to have the new tests run.

astrofrog commented 2 months ago

I've pushed an additional commit adding a test for the image viewer. I've also removed subset_state as a 'modifier' for now as it isn't really needed - this is for if we wanted to link the subset state to one that exists as a callback properly on a state class, which we don't really use. It's also not clear if modifying the subset state in-place would result in the correct events being fired. So for now, as it isn't needed, I decided to remove/simplify this.

@dhomeier - are you happy with these changes? If so, I think we should be good to go assuming the CI passes.

dhomeier commented 2 months ago

Thanks for the updates and new tests. When I initially worked on this, I thought the option to pass any subset_state (not just a slices one) accepted by compute_statistic through could prove useful, but obviously there is no immediate application for that. Checked functionality with Cubeviz again, all looking good!

astrofrog commented 2 months ago

Yes I agree it could be useful to allow arbitrary subsets - but it will be a bit more work as we need to figure out how to watch for subset updates.