Run trajectory plots after analysis completes - #3466
Conversation
Store plotting options before starting the worker and defer `plot_trajectories(showfigures=True)` to a main-thread finished slot. The analysis pipeline now always runs with `showfigures=False`, only opens trajectory plots when analysis succeeds, and centralizes button/progress-bar reset in the completion handler.
There was a problem hiding this comment.
Pull request overview
This PR updates the Analyze Videos GUI workflow to avoid GUI freezes by ensuring trajectory plots are only displayed from the main (GUI) thread after background analysis completes successfully.
Changes:
- Stores plotting options/batches before launching the worker thread and defers interactive plotting to a finished-handler slot.
- Forces the analysis pipeline to run
plot_trajectories(..., showfigures=False)and only opens figures when analysis succeeds and the user requested plot display. - Centralizes UI reset (button/progress bar) in the completion handler.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Refactor post-analysis trajectory plot display to use a single-shot QTimer, ensuring the UI (button re-enable, progress bar hide) fully updates before plots are shown. Also fixes the `_handle_analysis_error` slot signature to accept the error argument, and adds error handling around trajectory plot display.
Update the Analyze Videos tab so `deeplabcut.plot_trajectories(...)` only runs when `plot_trajectories` is enabled and `show_trajectory_plots` is not. This prevents triggering the plotting routine in cases where trajectory plots are already being shown through the separate display flow.
Extract repeated None-assignment into `_clear_pending_trajectory_plots()` helper. Also move the button re-enable and progress bar hide after the plot timer/clear logic to ensure UI updates happen regardless of plot path.
deruyter92
left a comment
There was a problem hiding this comment.
Good PR!
Just one remark about a behavioral change. Not sure how relevant it is, but worth having your explicit opinion.
| @Slot() | ||
| def _handle_analysis_finished(self): | ||
| should_show_plots = ( | ||
| not self._analysis_failed | ||
| and self._pending_plot_options is not None | ||
| and self._pending_plot_batches is not None | ||
| and self._pending_plot_options.plot_trajectories | ||
| and self._pending_plot_options.show_trajectory_plots | ||
| ) | ||
|
|
||
| self._analysis_failed = False | ||
|
|
||
| if should_show_plots: | ||
| self._pending_plot_timer.start() | ||
| else: | ||
| self._clear_pending_trajectory_plots() |
There was a problem hiding this comment.
One behavioral change:
This PR moves the plotting step from during successful batch processing to after the whole pipeline succeeds. Now, a late failure such as CSV conversion can suppress plots that otherwise would have been shown.
Do yo think this is justified or should we keep the previous behavior where plots are visualized as each batch succeeds?
deruyter92
left a comment
There was a problem hiding this comment.
after our discussion, we decided that this needs further changing ideally. To not change the existing behavior.
Store plotting options before starting the worker and defer
plot_trajectories(showfigures=True)to a main-thread finished slot.The analysis pipeline now always runs with
showfigures=False, only opens trajectory plots when analysis succeeds, and centralizes button/progress-bar reset in the completion handler.This fixes the GUI freezing due to plots opening in the wrong thread.
Closes #3462.