-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
cubeviz spectral extraction live-previews #2733
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2733 +/- ##
==========================================
+ Coverage 88.67% 88.88% +0.21%
==========================================
Files 108 108
Lines 15886 15981 +95
==========================================
+ Hits 14087 14205 +118
+ Misses 1799 1776 -23 ☔ View full report in Codecov by Sentry. |
64381fb
to
a311743
Compare
a311743
to
cab02e2
Compare
95c201f
to
a1c5b44
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to work for me (FWIW I do hit the 0.3 second limit with the example cube, my computer must be lazier), I'm going to spend a little more time understanding all the code before approving.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good. Is there any way to add a test for the timeout? I don't see an easy way to do it since you can't set the timeout in the decorator dynamically/on the fly when calling the function, so you can't just set it to 0 for the test.
I guess we could maybe test it with a fake plugin that just sleeps for half a second? 🤷♂️ Is that worth doing? |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it in Cubeviz, and it behaves as described. Thank you for your work.
Description
This pull request implements live-previews for spectral extraction in cubeviz. Since this may become expensive and to mitigate potential lag, this also migrates the functionality to time expensive live-previews and temporarily disable them from lcviz to here as a decorator which is then used in the implementation of live-previews in cubeviz's spectral extraction.
lcviz can then remove its own implementation to make use of this PR in kecnry/lcviz#3
NOTE: this screen recording was done with the timeout lowered to 0.1 seconds to show how the pausing works. In the PR, this value is set to 0.3 and usually does not trigger for the default cube. The "entire cube" does not display any preview because of a known bug where nans in the cube are resulting in a spectrum of entire nans (addressed by #2737).
Screen.Recording.2024-03-01.at.11.28.51.AM.mov
Change log entry
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, maintainershould 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.
trivial
label.