Fix stretch histogram reset_limits

[astrofrog]

Description

This should fix reset_limits in the stretch histogram

Fixes #2506

Change log entry

• Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
  list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
  should add a no-changelog-entry-needed label.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

• Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
• Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
• Do the proposed changes follow the STScI Style Guides?
• Are tests added/updated as required? If so, do they follow the STScI Style Guides?
• Are docs added/updated as required? If so, do they follow the STScI Style Guides?
• Did the CI pass? If not, are the failures related?
• Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone.
• After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

[pllim]

@kecnry , should this be backported? The code for this plugin in v3.7.x look very different.

[pllim]

@astrofrog , do we need "hold sync" block here?

[pllim]

Now the CI failure is real:

E           glue.core.exceptions.IncompatibleAttribute: x

glue/core/data.py:1467: IncompatibleAttribute

[astrofrog]

I'll investigate the failure, it is indeed a real problem.

[kecnry]

should this be backported?

No need to backport this, no. The migration to using a glue viewer is unreleased.

[astrofrog]

I found a bug deep inside glue that was causing issues here:

• Fix bug when changing the number of bins on a histogram with modified/deleted data glue-viz/glue#2451

I've released glue-core v1.14.1 with a fix for that bug. I've now removed the workaround that I originally added in this PR and have also removed the workaround to add the 'ref' dataset at all as this appears to no longer be needed with the above bug fix.

One thing that is missing at the moment in a regression test for the original bug here - it might be better for @kecnry to add this as I'm not sure what the best way is to do this via public API.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0e8981c) 90.81% compared to head (ee38073) 90.80%.
Report is 20 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2529      +/-   ##
==========================================
- Coverage   90.81%   90.80%   -0.02%     
==========================================
  Files         160      160              
  Lines       19298    19394      +96     
==========================================
+ Hits        17525    17610      +85     
- Misses       1773     1784      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[astrofrog]

I removed the changelog entry and marked this as affects-dev

[kecnry]

I'm still seeing some weird behavior. Would you expect reset_limits to also affect viewer.set.hist_x_min/hist_x_max? But even once I reset those to the originally set value, the bins still don't look the same as they did before resetting the limits 🤔

[astrofrog]

@kecnry - I'll investigate!

[astrofrog]

@kecnry - from what I can see, the stretch histogram in the plot options manually sets the histogram limits to be the 95% percentile limits of the data. However, reset_limits as implemented on the state object for the histogram viewer uses the strict min/max of the data to set the histogram limits. In addition, reset_limits does cause state.update_bins_to_view to be called, so when you call state.reset_limits, it will set both x_min/x_max and hist_x_min/hist_x_max to be the strict min/max of the data.

So there are three questions here:

• Should we make it so that reset_limits in glue has the option to have a customizable percentile to use for the min/max? This would be easy enough to adjust in glue-core (see https://github.com/glue-viz/glue/blob/4de2cfd31670acb9aae60d198be4a5f83ebd8c0e/glue/viewers/histogram/state.py#L70) as we can simply set the percentile to a variable instead of 100%. This would then avoid jdaviz having to implement custom percentile interval logic and would make the home button work as you expect.

• Do you want reset_limits to not reset the bins being used, or would this be fine provided the bins were also reset to e.g. the 95% percentile limits if we implement the above?

• Do you think there is another issue going on not described by the above?

[kecnry]

Should we make it so that reset_limits in glue has the option to have a customizable percentile to use for the min/max? This would be easy enough to adjust in glue-core (see https://github.com/glue-viz/glue/blob/4de2cfd31670acb9aae60d198be4a5f83ebd8c0e/glue/viewers/histogram/state.py#L70) as we can simply set the percentile to a variable instead of 100%. This would then avoid jdaviz having to implement custom percentile interval logic and would make the home button work as you expect.

This would definitely be handy for our use-case if you think it is something useful upstream. Otherwise, I think it is reasonable that the x-limits would be reset to contain all the bins, but I don't think I'd expect the home button to reset the bins, but rather only x/y-limits. Alternatively I guess we could just call the internal reset_x/y_limits methods if you think the general case should reset the bins?

[pllim]

The 95th percentile rings a bell. I think POs wanted the default to be 95th percentile or something because min/max was not displaying anything meaningful in real data.

[kecnry]

That's a good point - we may want bypass the home button eventually to set the limits based on the current locations of vmin/vmax, but I think reset_limits still needs to be fixed either way.

[astrofrog]

@kecnry - see glue-viz/glue#2455, with this you can do:

po._obj.stretch_histogram.viewer.state.update_bins_on_reset_limits = False
po._obj.stretch_histogram.viewer.state.x_limits_percentile = 95

which I think would give the behavior you want. Could you test it out and let me know if this meets your needs? If so, I can merge & release.

[kecnry]

@astrofrog - that seems to do the trick and act as expected thanks! I added a commit here which will make use of that once its release and the pin for glue updated in this PR - will that be 1.14.2 or 1.15?