Speed up histogram in plot options (alternative to #2763)

[astrofrog]

Description

Like #2763, this starts off by reverting #2735, and then makes use of:

• Add the ability to specify a random subset size for computing histogram glue-viz/glue#2478
• Expose ability to use a random subset of data for computing histograms glue-viz/glue-jupyter#424

to speed up the histogram calculation. I also removed:

        # filter out nans (or else bqplot will fail)
        if np.any(np.isnan(sub_data)):
            sub_data = sub_data[~np.isnan(sub_data)]

which was causing the whole array to get loaded into memory - this isn't needed anyway because glue's histogram viewer deals fine with NaN values. Finally, I also updated the calculation of the percentile limits to use glue, also with random sampling, otherwise PercentileLimits caused the whole array to be loaded into memory and caused slowdowns.

With these changes, I was able to load an 8Gb memory-mapped array and have the histogram update within a second or so. We might want to play with the random subset size a little to see what is a good compromise with real-world data.

I wonder whether it might make sense to consider having some kind of performance benchmark for this kind of functionality, because it's easy to accidentally introduce regressions that cause all the data to be used.

This requires the two glue PRs to be merged and released before merging this PR.

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. Bugfix milestone also needs an accompanying backport label.
• After merge, any internal documentations need updating (e.g., JIRA, Innerspace)? 🐱

[pllim]

Hmm. I installed those upstream branches you mentioned and installed Jdaviz from this PR branch. After I load that 8 GB image from Cami, I saw a non-sensible histogram for a second (just one bar) and then it self-refreshed and got stuck in the loading circle of death. All I did was load that image and then open Plot Options plugin, scroll down, and waited.

[pllim]

Also looks like this patch broke the spline thingy?

>           assert np.allclose(po._obj.stretch_params_value["knots"], knots_after_drag_move)
E           assert False
E            +  where False = <function allclose at 0x000001FC915F9170>(([0.0, 0.1, 0.21712585033417825, 0.7, 1.0], [0.0, 0.05, 0.3097925111911964, 0.9, 1.0]), ([0.0, 0.1, 0.21712585033417825, 0.7, 1.0], [0.0, 0.05, 0.2900993441358025, 0.9, 1.0]))
E            +    where <function allclose at 0x000001FC915F9170> = np.allclose

jdaviz\core\tests\test_tools.py:103: AssertionError

[astrofrog]

@pllim could you send me a link to the 8Gb file? Thanks!

[pllim]

@astrofrog , Cami has reached out to you separately about the file. FYI.

[astrofrog]

@pllim - could you try and run the following Python script using pyinstrument and let me know the output?

from jdaviz import Imviz

imviz = Imviz()
viewer = imviz.default_viewer

imviz.load_data("egs_all_acs_wfc_f814w_030mas_v1.9_drz.fits")
imviz.plugins["Plot Options"]._obj._update_stretch_histogram()

With this PR and the two glue ones, the total time for the script goes down from 40s to 10s and a lot of that is the loading of the FITS file which has 27634 entries in the header?

[pllim]

could you try and run the following Python script using pyinstrument and let me know the output?

I tried calling pyinstrument script.py but it is giving me unrelated errors like this from astropy.units:

RuntimeError: dictionary changed size during iteration

And after I hack-patched those on the installed astropy, it gave me this from a package I know nothing about:

toolz/curried/operator.py", line 15, in <module>
    {name: f if name in IGNORE else curry(f)
                                    ^^^^^^^^
  File "python3.12/site-packages/toolz/functoolz.py", line 201, in __init__
    raise TypeError("Input must be callable")
TypeError: Input must be callable

Am I doing it wrong? Please advise. Thanks!

[pllim]

FWIW, with this PR's current state, rebased on top of main, I still see the "loading circle of death".

[astrofrog]

@pllim huh very weird - could you let me know what you get for:

pyinstrument --version

Thanks!

[pllim]

This should not be deleted.

[astrofrog]

Oops

[astrofrog]

Fixed

[astrofrog]

@pllim - ready for another review

[pllim]

Thanks! LGTM.

[rosteen]

Thanks! LGTM. Edit: Wow I didn't realize this was character-for-character exactly what @pllim said 😆