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

improve stretch histogram performance (downsampling and decorator) #2408

Merged
merged 5 commits into from
Aug 31, 2023

Conversation

kecnry
Copy link
Member

@kecnry kecnry commented Aug 30, 2023

Description

This pull request improves the performance of the stretch histogram and the stretch curve, especially for large images, by downsampling the data sent to the histogram and by making use of #2386 for the stretch curve introduced in #2390.

Screen.Recording.2023-08-31.at.8.15.01.AM.mov

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)?

@kecnry kecnry added this to the 3.7 milestone Aug 30, 2023
Comment on lines +600 to +603
if size > 400**2:
xstep = max(1, round(arr.shape[1] / 400))
ystep = max(1, round(arr.shape[0] / 400))
arr = arr[::ystep, ::xstep]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pllim - this code block essentially exists in three places in jdaviz now (compass and twice here). Do you think it's worth moving somewhere reusable? Maybe in utils?

Copy link
Contributor

@pllim pllim Aug 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure. If you want this to turn into utility function, then you have to make sure that function is not making accidental copies (everything must pass by reference where it matters and must return a view) and maybe a way to adjust sampling factor and maybe even indices (for generic ndim support). Something to think about.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to refactor in this PR? If so, I will wait before reviewing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had some of those same concerns, so let's leave this as-is for now and consider moving the logic up higher if we need it anywhere else in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a bad feeling that we might have to revisit this as more and more new analysis stuff get requested. Should we open a ticket pre-emptively for the future?

@kecnry kecnry marked this pull request as ready for review August 30, 2023 19:46
@codecov
Copy link

codecov bot commented Aug 30, 2023

Codecov Report

Patch coverage is 48.14% of modified lines.

Files Changed Coverage
...nfigs/default/plugins/plot_options/plot_options.py 48.14%

📢 Thoughts on this report? Let us know!.

@camipacifici
Copy link
Contributor

camipacifici commented Aug 30, 2023

Great improvement, thank you!

@pllim pllim added performance Performance related bug Something isn't working labels Aug 30, 2023
Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is unavoidable to decrease the accuracy of histogram to make it not lag so much. I tried comparing like this and while I see some difference, it is good enough. This is an example using the first image in Imviz Example Notebook when zoomed in and only show data within display window.

Screenshot 2023-08-30 171732

# Approx X/Y limits from imviz.default_viewer.state.x/y_min/max
pf = fits.open(f'{data_dir}/jw02727-o002_t062_nircam_clear-f090w_i2d.fits')
plt.hist(pf[1].data[2131:3523, 1699:3244].ravel(),
         bins=25, range=(0, 0.5852748259901993),
         rwidth=0.9, color="gray")
plt.axvline(0, color="red")
plt.axvline(0.5852748259901993, color="red")

Screenshot 2023-08-30 172236

But can we make the blue info box a little smaller?

Screenshot 2023-08-30 170349

@pllim
Copy link
Contributor

pllim commented Aug 30, 2023

So does histogram even need Y tick marks at this point? The numbers on Y axis do not mean much here.

@kecnry
Copy link
Member Author

kecnry commented Aug 31, 2023

So does histogram even need Y tick marks at this point? The numbers on Y axis do not mean much here.

They are normalized density, so I wouldn't say they're meaningless, but I'd also be in favor of removing them since I think that would be assumed (or we can keep the label, but remove the numbers).

But can we make the blue info box a little smaller?

I don't want to set specific rules for this alert that differs from the rest.... but let me try alternate options for how to display this information. Maybe even in the title of the plot (so that it also shows in the plot popout...)? EDIT: see the latest commit and updated video in the PR description.

image

* takes less space and shows in popout
@pllim
Copy link
Contributor

pllim commented Aug 31, 2023

They are normalized density, so I wouldn't say they're meaningless

Not much meaning when it is subsampling. I guess it is fine if you want to keep the numbers but I fear they might confuse more than help. Unless you want to normalize to max of 1. Then it can be like percentage but in decimal form.

Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cami is happy. Code looks like code.

Y-axis discussions is technically out of scope, so approving. Thanks!

@kecnry kecnry merged commit 60ddb58 into spacetelescope:main Aug 31, 2023
@kecnry kecnry deleted the stretch-hist-performance branch August 31, 2023 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working performance Performance related Ready for final review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants