Refactor LossViewer to use matplotlib
#1899
Conversation
…ssViewer` (untested)
WalkthroughThe recent changes enhance the Sleap application's graphical representation by integrating Matplotlib as the primary plotting framework while retaining QtCharts functionality. This transition includes the introduction of a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LossViewer
participant LossPlot
User->>LossViewer: Request to visualize loss data
LossViewer->>LossPlot: Create LossPlot instance
LossPlot-->>LossViewer: Return plot
LossViewer->>LossPlot: Add training/validation loss data
LossPlot-->>LossViewer: Update visualization
LossViewer->>User: Display updated loss chart
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- sleap/gui/app.py (1 hunks)
- sleap/gui/widgets/monitor.py (8 hunks)
- sleap/gui/widgets/mpl.py (1 hunks)
Files skipped from review due to trivial changes (1)
- sleap/gui/app.py
Additional context used
Ruff
sleap/gui/widgets/monitor.py
837-838: Use a single
ifstatement instead of nestedifstatements(SIM102)
Additional comments not posted (4)
sleap/gui/widgets/mpl.py (2)
9-9: LGTM: Import statement updated for new backend.The import statement for
FigureCanvasQTAgghas been updated to align with theQtAggbackend. This change is appropriate for compatibility with PySide 6.
12-12: LGTM: Backend configuration updated toQtAgg.The Matplotlib backend has been set to
QtAgg, which is consistent with the import update and should improve compatibility with PySide 6.sleap/gui/widgets/monitor.py (2)
21-493: LGTM:LossPlotclass implementation.The
LossPlotclass is well-structured and effectively encapsulates the logic for plotting loss data using Matplotlib. The methods for initializing series, setting up axes, and managing plot updates are clear and appropriate for the intended functionality.
Line range hint
495-928: LGTM:LossViewerclass refactoring.The refactoring of the
LossViewerclass to use theLossPlotclass improves the clarity and modularity of the code. The integration of Matplotlib enhances the visual representation of training metrics.Tools
Ruff
837-838: Use a single
ifstatement instead of nestedifstatements(SIM102)
| if which == "val_loss": | ||
| if self.best_val_y is None or y < self.best_val_y: |
There was a problem hiding this comment.
Simplify nested if statement.
The nested if statement can be simplified to improve readability.
Apply this diff to simplify the condition:
- if which == "val_loss":
- if self.best_val_y is None or y < self.best_val_y:
+ if which == "val_loss" and (self.best_val_y is None or y < self.best_val_y):Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if which == "val_loss": | |
| if self.best_val_y is None or y < self.best_val_y: | |
| if which == "val_loss" and (self.best_val_y is None or y < self.best_val_y): |
Tools
Ruff
837-838: Use a single
ifstatement instead of nestedifstatements(SIM102)
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (3)
sleap/gui/widgets/monitor.py (3)
Line range hint
586-619: Extract ZMQ port initialization to a separate method.The initialization of ZMQ ports is embedded within the constructor. Consider extracting this logic into a separate method for clarity and maintainability.
Here's a suggestion to refactor the ZMQ port initialization:
def initialize_zmq_ports(self, zmq_ports): zmq_ports = zmq_ports or dict() zmq_ports["publish_port"] = zmq_ports.get("publish_port", 9001) zmq_ports["controller_port"] = zmq_ports.get("controller_port", 9000) return zmq_ports # Usage in __init__ self.zmq_ports = self.initialize_zmq_ports(zmq_ports)
Line range hint
658-750: Extract layout setup to a separate method.The layout setup logic is embedded within the
resetmethod. Consider extracting this logic into a separate method for clarity and maintainability.Here's a suggestion to refactor the layout setup:
def setup_layout(self): layout = QtWidgets.QVBoxLayout() layout.addWidget(self.canvas) if self.show_controller: control_layout = QtWidgets.QHBoxLayout() # Add control elements to control_layout layout.addLayout(control_layout) wid = QtWidgets.QWidget() wid.setLayout(layout) self.setCentralWidget(wid) # Usage in reset self.setup_layout()
Line range hint
802-876: Extract free port finding logic to a utility function.The logic for finding a free port is embedded within the
setup_zmqmethod. Consider extracting this logic into a utility function for reuse and clarity.Here's a suggestion to refactor the free port finding logic:
def find_free_port(port: int, zmq_context: zmq.Context, max_attempts: int = 10): attempts = 0 while not is_port_free(port=port, zmq_context=zmq_context): if attempts >= max_attempts: raise RuntimeError( f"Could not find free port after {max_attempts} attempts." ) port = select_zmq_port(zmq_context=zmq_context) attempts += 1 return port # Usage in setup_zmq self.zmq_ports["publish_port"] = find_free_port( port=self.zmq_ports["publish_port"], zmq_context=self.ctx )
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sleap/gui/widgets/monitor.py (8 hunks)
Additional context used
Ruff
sleap/gui/widgets/monitor.py
928-929: Use a single
ifstatement instead of nestedifstatements(SIM102)
Additional comments not posted (25)
sleap/gui/widgets/monitor.py (25)
121-137: LGTM!The
log_scaleproperty is well-implemented and correctly toggles the y-axis scale.
177-208: LGTM!The
_set_title_spacemethod is well-implemented and effectively calculates the title space.
210-225: LGTM!The
_setup_x_axismethod is well-implemented and correctly configures the x-axis.
227-247: LGTM!The
_set_up_y_axismethod is well-implemented and correctly configures the y-axis.
249-274: LGTM!The
_setup_legendmethod is well-implemented and effectively configures the legend.
276-292: LGTM!The
_setup_major_gridlinesmethod is well-implemented and effectively configures the gridlines and spines.
294-309: LGTM!The
_add_midpoint_gridlinesmethod is well-implemented and effectively adds midpoint gridlines.
311-326: LGTM!The
set_data_on_scattermethod is well-implemented and correctly sets data on scatter plots.
327-342: LGTM!The
add_data_to_plotmethod is well-implemented and correctly adds data to line plots.
343-359: LGTM!The
calculate_xlimmethod is well-implemented and correctly calculates x-axis limits.
361-393: LGTM!The
calculate_ylimmethod is well-implemented and correctly calculates y-axis limits.
395-417: LGTM!The
resize_axesmethod is well-implemented and correctly resizes axes to fit data.
419-422: LGTM!The
redraw_plotmethod is well-implemented and correctly redraws the plot.
750-765: LGTM!The
log_scaleproperty is well-implemented and correctly toggles the y-axis scale.
767-781: LGTM!The
ignore_outliersproperty is well-implemented and correctly manages outlier handling.
783-785: LGTM!The
toggle_ignore_outliersmethod is well-implemented and correctly toggles outlier handling.
787-789: LGTM!The
toggle_log_scalemethod is well-implemented and correctly toggles the y-axis scale.
Line range hint
791-800: LGTM!The
set_batches_to_showmethod is well-implemented and correctly manages the number of batches to display.
Line range hint
878-884: LGTM!The
cancelmethod is well-implemented and correctly manages the cancel action.
Line range hint
886-896: LGTM!The
stopmethod is well-implemented and correctly manages the stop action.
Line range hint
898-936: Simplify nested if statement forval_loss.The nested if statement for
val_losscan be simplified to improve readability.Apply this diff to simplify the condition:
- if which == "val_loss": - if self.best_val_y is None or y < self.best_val_y: + if which == "val_loss" and (self.best_val_y is None or y < self.best_val_y):Tools
Ruff
928-929: Use a single
ifstatement instead of nestedifstatements(SIM102)
938-952: LGTM!The
_set_data_on_scattermethod is well-implemented and correctly sets data on scatter plots.
953-967: LGTM!The
_add_data_to_plotmethod is well-implemented and correctly adds data to line plots.
968-971: LGTM!The
_redraw_plotmethod is well-implemented and correctly redraws the plot.
973-982: LGTM!The
_resize_axesmethod is well-implemented and correctly resizes axes to fit data.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sleap/gui/widgets/monitor.py (8 hunks)
Additional context used
Ruff
sleap/gui/widgets/monitor.py
928-929: Use a single
ifstatement instead of nestedifstatements(SIM102)
Additional comments not posted (5)
sleap/gui/widgets/monitor.py (5)
22-445: Well-structuredLossPlotclass for Matplotlib integration.The
LossPlotclass is well-structured and effectively encapsulates the functionality required for plotting training and validation loss curves using Matplotlib. The methods are well-organized, and the class properties are used effectively to manage state.
Line range hint
586-1028: Effective integration ofLossPlotinLossViewer.The refactoring of the
LossViewerclass to use the newLossPlotclass for Matplotlib plotting is effectively done. The methods related to data handling, ZMQ integration, and GUI controls are well-implemented. The use of properties for managing plot settings likelog_scaleandignore_outliersis a good practice.Tools
Ruff
928-929: Use a single
ifstatement instead of nestedifstatements(SIM102)
953-967: Ensure separation of concerns in_add_data_to_plot.The method
_add_data_to_plotcorrectly handles adding data to line plots and ensures it does not interact with scatter plots. This separation of concerns is crucial for maintaining clarity and correctness in the plotting logic.
938-952: Correct implementation of_set_data_on_scatter.The method
_set_data_on_scatteris well-implemented, focusing solely on scatter plots and ensuring no interaction with line plots. This method effectively manages the data setting for scatter plots, which is essential for the correct visualization of batch and best validation losses.
789-789: Ensure propagation of log scale setting intoggle_log_scale.The method
toggle_log_scaleeffectively toggles the log scale setting. It is crucial to ensure that this setting is correctly propagated to theLossPlotinstance to maintain consistency in the plot appearance.
Otherwise we get an unnecessary teminal message: UserWarning: Attempting to set identical bottom == top == 3.0 results in singular transformations; automatically expanding. self.axes.set_ylim(y_min, y_max)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sleap/gui/widgets/monitor.py (8 hunks)
Additional context used
Ruff
sleap/gui/widgets/monitor.py
929-930: Use a single
ifstatement instead of nestedifstatements(SIM102)
Additional comments not posted (2)
sleap/gui/widgets/monitor.py (2)
22-199: Comprehensive review ofLossPlotclass.The
LossPlotclass is well-structured and encapsulates all functionalities related to the plotting of training and validation loss curves using Matplotlib. The methods are well-organized, and the use of properties forlog_scaleand handling of outliers is appropriate.
- Initialization and Setup: The constructor and setup methods (
__init__,_setup_x_axis,_setup_y_axis,_setup_legend,_setup_major_gridlines) are correctly implemented. The use of properties to handle attributes likelog_scaleandignore_outliersis a good practice, ensuring that changes to these attributes are handled consistently.- Data Handling: Methods like
set_data_on_scatterandadd_data_to_plotare clear and concise, appropriately checking the type of plot before adding data.- Axes and Gridlines Management: The methods handling axes adjustments (
resize_axes,_calculate_xlim,_calculate_ylim) and gridline setup are crucial for ensuring the plot scales correctly with incoming data. The implementation here handles these aspects well, with considerations for log scale and outlier handling.- Redrawing and Title Updates: The
redraw_plotandupdate_runtime_titlemethods ensure that the plot updates are efficient and that runtime information is displayed accurately.Overall, the
LossPlotclass is a robust implementation for the plotting needs of theLossViewerand integrates well with the existing architecture. The use of Matplotlib is effectively leveraged to enhance the visualization capabilities.
Line range hint
587-1020: Detailed review ofLossViewerclass.The
LossViewerclass handles the GUI and integration of theLossPlotfor displaying training metrics. The class is well-structured, and the methods are appropriately segmented to handle different aspects of the GUI and data handling.
- Initialization and Reset: The constructor and
resetmethod are well-implemented, ensuring that theLossPlotis correctly instantiated and configured. The handling of ZMQ for communication is robust, providing a solid foundation for receiving training data.- Data Handling and Plotting: The methods
add_datapoint,_set_data_on_scatter, and_add_data_to_ploteffectively manage the data received during training and ensure it is plotted correctly. The separation of scatter and line plot handling is a good practice.- GUI Components: The setup of GUI components like checkboxes and buttons is done well, providing necessary controls to the user for adjusting plot settings.
- Runtime Updates: The method
update_runtimeand associated helper methods ensure that the plot's title and other runtime information are updated accurately based on the training progress.Overall, the
LossViewerclass effectively integrates theLossPlotand manages the GUI components necessary for displaying training metrics. The class is a key part of the application's functionality, allowing users to monitor training progress interactively.Tools
Ruff
929-930: Use a single
ifstatement instead of nestedifstatements(SIM102)
Description
In a downstream branch #1841, we upgrade all of the dependencies to use Python 3.10. However,
QtChartsis problematic--it is missing libraries in updatedPySide6We are only using
QtChartsin theLossViewer, so, in this PR, we replaceQtChartswithmatplotlib. ALossPlotclass that subclassesMplCanvasis added to handle the plotting functionality previously mixed intoLossViewer. The unused duplicate of theLossViewerin training_monitor.py is removed. To maintain scope, the refactoredLossViewerre-uses same logic and only replacesQtChartswithmatplotlib.LossViewer, we came across a "leaked semaphore" error that seemed to occur only for (a?) bottom-up model(s?) on (a?) Mac M2(s?). This issue will be further investigated, but has been deemed unrelated to the refactoring in this PR as it was replicated on the base develop branch as well (and seems more likely to be model/configuration related).Types of changes
Does this address any currently open issues?
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor