flatten/binning: handle slow live-updates for large data #56
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* raise a warning alert in the plugin telling the user the live-previews aren't updating and prompt them to manually update the preview or to disable the preview markers
* doesn't require upstream PR, but won't have any affect without it
Codecov ReportAttention:
📢 Thoughts on this report? Let us know! |
bmorris3
approved these changes
Nov 27, 2023
| @@ -189,6 +200,10 @@ def _live_update(self, event={}): | |||
| mark.times = [] | |||
| mark.update_xy(times, lc.flux.value) | |||
|
|
|||
| self.last_live_time = np.round(time() - start, 2) | |||
| if self.last_live_time > 0.3: | |||
There was a problem hiding this comment.
Can we make the threshold a traitlet, and expose it in the user API? It'd be great if a user could assert: "I'm sick of that warning, I'll gladly wait, leave me alone." The user could set the number to 10_000, get coffee, and come back without a warning.
There was a problem hiding this comment.
I like this idea, but even though we aren't technically committing to API yet, we might soon, and I think it might make sense to play with it and converge more on the feature (if necessary) before we make it customizable.
bmorris3
pushed a commit
to bmorris3/lcviz
that referenced
this pull request
Dec 8, 2023
…ope#56) * flatten: disable live-updating when slow * raise a warning alert in the plugin telling the user the live-previews aren't updating and prompt them to manually update the preview or to disable the preview markers * binning: disable live-updating when slow * make use of action_spinner * doesn't require upstream PR, but won't have any affect without it
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces a check on the time that the live-updates take, and if exceeding a certain threshold, temporarily disables the live-preview updates and shows an alert prompting to either manually update the live-preview or disable the live-preview marks.
This also makes use of a change upstream in jdaviz to show a spinner in the "apply" buttons when the plugin is computing which temporarily disables clicking the button. For cases where binning/flattening is slow to run, this gives immediate feedback that the button was clicked and doesn't need to be clicked again. This doesn't need to be blocked by pinning a new release of jdaviz,
but should make sure that PR gets merged first.The code here can be simplified slightly by using the
@with_spinner()decorator from spacetelescope/jdaviz#2560, but that can be a follow-up once that is in a jdaviz release.Note: this video was before some style changes in spacetelescope/jdaviz#2560 - the check marks are now gone and will show a spinner, if running on jdaviz main. If not running on main, the "bin" button will not react, but everything else will still work.
Screen.Recording.2023-11-10.at.12.35.36.PM.mov