Simplifying MultiPlot Interface #83 #87
Conversation
WalkthroughThis pull request enhances plotting capabilities across multiple modules. Gallery scripts now automate subplot creation using a new Changes
Sequence Diagram(s)sequenceDiagram
participant U as User/Application
participant CP as ChromatogramPlot
participant DF as DataFrame
U->>CP: Call plot()
CP->>CP: _validate_plot_config()
alt Valid facet_column
CP->>DF: Group data by facet_column
DF-->>CP: Return grouped data
CP->>CP: Create subplots and prepare tooltips
else Invalid facet_column
CP->>CP: Fallback to single plot logic
end
CP-->>U: Display plot with subplots
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py (2)
12-12: Remove unused import.Static analysis indicates
sysis never used in this script. Consider removing it:- import sys🧰 Tools
🪛 Ruff (0.8.2)
12-12:
sysimported but unusedRemove unused import:
sys(F401)
31-32: Catching broadException.Catching the base
Exceptioncan mask unexpected errors. Using a more specific exception (e.g.,requests.exceptions.RequestExceptionfor downloads, orzipfile.BadZipFile) helps with clarity and debugging.Also applies to: 39-40
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py(2 hunks)pyopenms_viz/_config.py(1 hunks)pyopenms_viz/_core.py(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py (1)
pyopenms_viz/_core.py (5)
plot(347-351)plot(581-654)plot(692-694)plot(794-892)plot(1215-1227)
🪛 Ruff (0.8.2)
pyopenms_viz/_core.py
19-19: Redefinition of unused np from line 5
Remove definition: np
(F811)
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py
11-11: matplotlib.pyplot imported but unused
Remove unused import: matplotlib.pyplot
(F401)
12-12: sys imported but unused
Remove unused import: sys
(F401)
🔇 Additional comments (7)
pyopenms_viz/_config.py (1)
208-210: Looks good!Adding
tile_by,tile_columns, andtile_figsizeto the base configuration is consistent with the rest of the design, providing flexible tiling support.docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py (4)
2-5: Docstring enhancements look fine.The short introduction underlines the
tile_byparameter purpose effectively.
42-44: Data loading looks consistent.The paths and separators are specified clearly.
46-48: Comment updates are clear.The remark illustrates the shift to a single plot call using
tile_by.
49-68: Plot call withtile_byis straightforward.Nice usage of the new tiling mechanism. The parameters are self-explanatory and well-set here.
pyopenms_viz/_core.py (2)
580-655: Tile-by logic implementation is clean.The grouping, subplot creation, and annotation handling in
plot()are well-designed. Usinggroup_dffor each subplot is clear, and calling_add_peak_boundaries(group_annotations)ensures correct annotation distribution.
659-665: Docstring clarifications are fine.Routing developers to override
_add_peak_boundariesorcompute_apex_intensityif needed is commendable. This maintains flexibility for custom logic.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py (2)
11-11: Remove unused importThe
matplotlib.pyplotimport is not directly used in this script, as the plotting is handled through the pandas plotting backend.-import matplotlib.pyplot as plt🧰 Tools
🪛 Ruff (0.8.2)
11-11:
matplotlib.pyplotimported but unusedRemove unused import:
matplotlib.pyplot(F401)
32-32: Consider more specific exception handlingWhile simplified exception handling is acceptable for a demo script, catching specific exceptions would provide better error information to users.
-except Exception as e: +except (requests.RequestException, IOError) as e:-except Exception as e: +except zipfile.BadZipFile as e:Also applies to: 40-40
pyopenms_viz/_core.py (4)
585-597: Add error validation for tile_by parameterWhen the
tile_bycolumn doesn't exist in the annotation data but exists in the main data, this could lead to confusing output or errors. Consider adding validation to ensure both datasets have the column when necessary.# Check for tiling functionality tile_by = self._config.tile_by if hasattr(self._config, "tile_by") else None -if tile_by and tile_by in self.data.columns: +if tile_by and tile_by in self.data.columns: + # Validate that annotation data also has the tile_by column if it exists + if self.annotation_data is not None and tile_by not in self.annotation_data.columns: + warnings.warn(f"tile_by column '{tile_by}' not found in annotation data. Annotations will not be filtered properly.") + # Group the data by the tile_by column grouped = self.data.groupby(tile_by)
599-626: Consider adding figure title and overall stylingWhile individual subplot titles are set, there's no overall figure title that explains what the entire grid represents. Consider adding a figure title and more controls for the overall figure styling.
# Create a figure with a grid of subplots fig, axes = plt.subplots(tile_rows, tile_columns, figsize=figsize, squeeze=False) + +# Add a figure-level title if available +if hasattr(self._config, "title") and self._config.title: + fig.suptitle(self._config.title, fontsize=self._config.title_font_size if hasattr(self._config, "title_font_size") else 16) + axes = axes.flatten() # Easier indexing for a 1D list
628-631: Add null check before filtering annotationsIf
self.annotation_datais not None but empty, or if the tile_by column exists but has no matching values, this could lead to empty annotations or filtering issues.# If you have annotations that should be split, filter them too if self.annotation_data is not None and tile_by in self.annotation_data.columns: - group_annotations = self.annotation_data[self.annotation_data[tile_by] == group_val] - self._add_peak_boundaries(group_annotations) + group_annotations = self.annotation_data[self.annotation_data[tile_by] == group_val] + if not group_annotations.empty: + self._add_peak_boundaries(group_annotations)
633-636: Improve subplot handling with large datasetsWhen the number of groups is large, creating many subplots could impact performance and legibility. Consider adding a parameter to limit the number of subplots or paginate.
# Remove any extra axes if the grid size is larger than the number of groups for j in range(i + 1, len(axes)): fig.delaxes(axes[j]) +# Adjust layout with more control over spacing when there are many subplots +if num_groups > 9: + fig.tight_layout(pad=0.4, h_pad=1.0, w_pad=1.0) +else: + fig.tight_layout() -fig.tight_layout()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py(2 hunks)pyopenms_viz/_core.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py
11-11: matplotlib.pyplot imported but unused
Remove unused import: matplotlib.pyplot
(F401)
🔇 Additional comments (4)
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py (1)
47-69: Nice job implementing the tile_by parameter!The implementation of the
tile_byparameter makes the code much cleaner and easier to understand compared to manually creating subplots. The approach demonstrates good usage of the new parameter along with related options liketile_columns.pyopenms_viz/_core.py (3)
17-19: Necessary imports for the new tiling functionalityThe added imports for matplotlib and the math.ceil function are required for implementing the new tiling functionality. These are appropriate additions.
582-654: Excellent implementation of the tile_by feature!The implementation of the tiling functionality is well-structured and provides a significant improvement to the user experience by automating subplot creation. The code properly handles different configurations and gracefully falls back to the original behavior when tiling isn't specified.
656-659: Good docstring improvementThe updated docstring for the
_add_peak_boundariesmethod clearly indicates that it can be overridden, which is helpful for developers extending this class.
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py (2)
11-12: Remove unused imports.The imports for
matplotlib.pyplotandsysare not being directly used in the script. While pandas uses matplotlib internally, it's not explicitly referenced in your code.import pandas as pd import requests import zipfile -import matplotlib.pyplot as plt -import sys🧰 Tools
🪛 Ruff (0.8.2)
11-11:
matplotlib.pyplotimported but unusedRemove unused import:
matplotlib.pyplot(F401)
12-12:
sysimported but unusedRemove unused import:
sys(F401)
30-31: Consider using specific exception handlers instead of generic ones.While catching all exceptions with
Exceptionmakes the code more concise, it can mask different types of errors. Consider using specific exception types for better error handling and debugging.try: print(f"Downloading {zip_filename}...") response = requests.get(url) response.raise_for_status() # Check for any HTTP errors with open(zip_filename, "wb") as out: out.write(response.content) print(f"Downloaded {zip_filename} successfully.") -except Exception as e: +except requests.RequestException as e: + print(f"Error downloading zip file: {e}") +except IOError as e: print(f"Error downloading zip file: {e}") # Unzip the file try: with zipfile.ZipFile(zip_filename, "r") as zip_ref: zip_ref.extractall() print("Unzipped files successfully.") -except Exception as e: +except zipfile.BadZipFile as e: print(f"Error unzipping file: {e}")Also applies to: 38-39
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py (1)
pyopenms_viz/_core.py (5)
plot(346-350)plot(580-653)plot(691-693)plot(793-891)plot(1214-1226)
🪛 Ruff (0.8.2)
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py
11-11: matplotlib.pyplot imported but unused
Remove unused import: matplotlib.pyplot
(F401)
12-12: sys imported but unused
Remove unused import: sys
(F401)
🔇 Additional comments (1)
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py (1)
48-67: Great implementation of the tile_by parameter!The new approach using
tile_bysignificantly simplifies the code by automating subplot creation based on therun_namecolumn. This is much cleaner than the previous approach of manually creating subplots and iterating over predefined run names.
…n SpectrumPlot)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pyopenms_viz/_core.py (2)
593-596: Consider adding validation for tiling parameters.While the implementation works well, it would be good to add validation to ensure that
tile_columnsis a positive integer and to handle cases where there are very few or very many groups that might create problematic subplot layouts.# Get tiling options from config tile_columns = self._config.tile_columns if hasattr(self._config, "tile_columns") else 1 +# Ensure tile_columns is positive +tile_columns = max(1, tile_columns) tile_rows = int(ceil(num_groups / tile_columns)) +# Cap the number of rows/columns for extremely large group counts +if tile_rows > 20: # Arbitrary limit - adjust as needed + warnings.warn(f"Large number of subplots ({tile_rows}x{tile_columns}) created. Consider filtering your data.") figsize = self._config.tile_figsize if hasattr(self._config, "tile_figsize") else (10, 15)
586-654: Consider refactoring duplicate tiling logic into a helper method.The tiling functionality implemented in ChromatogramPlot.plot() is very similar to what's implemented in SpectrumPlot.plot(). Consider extracting the common logic into a helper method to reduce code duplication and make future maintenance easier.
You could create a helper method like:
def _create_tiled_plots(self, grouped_data, renderer_factory_func, tooltip_entries_func, additional_plot_func=None): """ Create tiled plots based on grouped data. Parameters: ----------- grouped_data : DataFrameGroupBy Data grouped by the tile_by column renderer_factory_func : callable Function that returns a renderer for each group tooltip_entries_func : callable Function that generates tooltip entries for each group additional_plot_func : callable, optional Function to add additional elements to each subplot Returns: -------- matplotlib.figure.Figure The figure containing the tiled plots """ num_groups = len(grouped_data) tile_columns = self._config.tile_columns if hasattr(self._config, "tile_columns") else 1 tile_rows = int(ceil(num_groups / tile_columns)) figsize = self._config.tile_figsize if hasattr(self._config, "tile_figsize") else (10, 15) fig, axes = plt.subplots(tile_rows, tile_columns, figsize=figsize, squeeze=False) axes = axes.flatten() for i, (group_val, group_df) in enumerate(grouped_data): ax = axes[i] # Get tooltip entries tooltip_entries = tooltip_entries_func(group_df) tooltips, custom_hover_data = self._create_tooltips(tooltip_entries, index=False) # Create and render the plot plot_obj = renderer_factory_func(group_df) plot_obj.canvas = ax plot_obj.generate(tooltips, custom_hover_data) # Set the title ax.set_title(f"{grouped_data.keys[0]}: {group_val}", fontsize=14) # Call additional customization function if provided if additional_plot_func: additional_plot_func(ax, group_df, group_val) # Remove extra axes for j in range(i + 1, len(axes)): fig.delaxes(axes[j]) fig.tight_layout() return figThis would allow you to simplify both plot methods significantly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/gallery_scripts_template/plot_investigate_spectrum_binning_ms_matplotlib.py(1 hunks)pyopenms_viz/_core.py(3 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
docs/gallery_scripts_template/plot_investigate_spectrum_binning_ms_matplotlib.py (1)
pyopenms_viz/_core.py (3)
canvas(125-126)canvas(129-130)show(397-403)
pyopenms_viz/_core.py (3)
pyopenms_viz/_bokeh/core.py (8)
fig(53-54)fig(57-59)_create_tooltips(462-469)get_line_renderer(443-444)generate(229-242)_add_peak_boundaries(477-520)_modify_y_range(205-221)_interactive(48-49)pyopenms_viz/_plotly/core.py (8)
fig(40-41)fig(44-46)_create_tooltips(547-568)get_line_renderer(533-534)generate(129-142)_add_peak_boundaries(573-603)_modify_y_range(226-235)_interactive(49-50)pyopenms_viz/_matplotlib/core.py (8)
ax(40-41)ax(44-46)_create_tooltips(492-494)get_line_renderer(474-475)generate(231-241)_add_peak_boundaries(503-569)_modify_y_range(197-213)_interactive(49-50)
🔇 Additional comments (9)
pyopenms_viz/_core.py (7)
17-19: Good addition of matplotlib imports for the new tiling functionality.These imports are necessary for the new subplot tiling feature that lets users visualize data grouped by specific columns.
580-584: Good docstring update explaining the new tiling functionality.The updated docstring clearly explains that the plot method now supports creating subplots based on unique values in a specified column, which aligns well with the PR objective of simplifying the MultiPlot interface.
585-587: Clean implementation of the tile_by feature check.The code safely checks for the existence of the tile_by configuration option using hasattr, which prevents errors if the config doesn't have this attribute yet.
588-654: Well-structured implementation of tiling functionality with good fallback.The implementation effectively:
- Groups data by the tile_by column
- Creates an appropriate grid of subplots
- Plots each group on its own axis with proper titles
- Handles annotations per subplot when available
- Cleans up unused axes
- Falls back to the original single-plot behavior when tiling isn't specified
This maintains backward compatibility while adding the new feature.
632-636: Good cleanup of unused subplot axes.Properly removing unused axes when the grid is larger than needed prevents empty plots from being displayed, which improves the visualization quality.
655-659: Clear and concise docstring for _add_peak_boundaries.The simplified docstring now clearly indicates that this method prepares data for peak boundaries and can be overridden if needed.
795-865: Good implementation of tiling functionality for SpectrumPlot.The tiling implementation for SpectrumPlot follows the same pattern as ChromatogramPlot, providing consistent behavior across different plot types. The code handles grouping, subplot creation, annotations, and even mirror spectrum display in a tiled layout.
docs/gallery_scripts_template/plot_investigate_spectrum_binning_ms_matplotlib.py (2)
81-81: Good practice to set show_plot=False when using subplots.Adding the
show_plot=Falseparameter is good practice when manually creating a figure with subplots. This prevents each individual plot from being displayed immediately, allowing you to show the complete figure with all subplots at once.
89-89: Corrected method for displaying matplotlib figures.Changing from
fig.show()toplt.show()is the correct approach for displaying figures in matplotlib. This ensures the figure is properly rendered in various environments (interactive console, scripts, notebooks, etc.).
jcharkow
left a comment
jcharkow
left a comment
There was a problem hiding this comment.
Looks good so far please see comments above.
The main thing that needs changing is that the _core.py is abstracted across multiple plotting backends, thus you need to ensure not to include any matplotlib specific code, this should instead be in maptlotlib/core.py.
Please also test the Spectrum functionality. I've added a suggestion for how to do this above and post screenshots.
| Plot Spyogenes subplots ms_matplotlib using tile_by | ||
| ==================================================== | ||
| Here we show how we can plot multiple chromatograms across runs together | ||
| This script downloads the Spyogenes data and uses the new tile_by parameter to create subplots automatically. |
There was a problem hiding this comment.
Do not need to edit the documentation description because the idea is that the tile_by will be the default
| except requests.RequestException as e: | ||
| except Exception as e: |
There was a problem hiding this comment.
Why is this changed from a RequestException?
There was a problem hiding this comment.
I initially changed requests.RequestException to a generic Exception to handle unexpected errors while downloading the ZIP file. However, I later realized that the errors were mostly network-related, so I reverted the change.
|
|
||
| # Save the zip file to the current directory |
|
|
||
| # URL of the zip file |
| # Extract all files to the current directory | ||
| zip_ref.extractall() | ||
| print("Unzipped files successfully.") | ||
| except zipfile.BadZipFile as e: |
There was a problem hiding this comment.
Why is this changed from BadZipFile
There was a problem hiding this comment.
I encountered an error while downloading the ZIP file and wanted to understand the reason, but I forgot to revert the changes.
| This is not a complete method should be overridden by subclasses. | ||
| Args: | ||
| annotation_data (DataFrame): The feature data containing the peak boundaries. | ||
| Returns: | ||
| None | ||
| (Override this method if needed.) | ||
| """ | ||
| # compute the apex intensity |
pyopenms_viz/_core.py
Outdated
| reference_spectrum = self._prepare_data(self.reference_spectrum) | ||
|
|
||
| # Check if tiling is requested | ||
| tile_by = self._config.tile_by if hasattr(self._config, "tile_by") else None |
There was a problem hiding this comment.
see above, likely do not need this line
pyopenms_viz/_core.py
Outdated
| fig, axes = plt.subplots(tile_rows, tile_columns, figsize=figsize, squeeze=False) | ||
| axes = axes.flatten() |
There was a problem hiding this comment.
matplotlib specific code should not be here as this is abstracted. Add this functionality to the create_figure method instead
pyopenms_viz/_core.py
Outdated
| min_value = reference_spectrum[self.y].min() | ||
| min_padding = -0.2 | ||
| max_padding = 0.4 | ||
| self._modify_y_range((min_value, max_value), padding=(min_padding, max_padding)) |
There was a problem hiding this comment.
Seems to be a lot of repetitive code, is there a way to clean this up?
There was a problem hiding this comment.
We can simplify it by extracting the y-axis range and padding logic into a helper method (e.g. _compute_y_range_and_padding) that centralizes the computation and reduces code duplication.
pyopenms_viz/_core.py
Outdated
| min_value = reference_spectrum[self.y].min() | ||
| min_padding = -0.2 | ||
| max_padding = 0.4 | ||
| self._modify_y_range((min_value, max_value), padding=(min_padding, max_padding)) |
There was a problem hiding this comment.
You could test out plotSpectrum by adding a "Run" column and just duplicating the entries in TestSpectrumDf.tsv for each run group.
There was a problem hiding this comment.
We could test plotSpectrum by adding a "Run" column to the TestSpectrumDf.tsv file and duplicating the entries for each run group. This lets us verify that the grouping functionality works as expected when handling multiple runs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py (1)
12-15: Fix duplicate system imports.There are duplicate imports of the
sysmodule in lines 12 and 13, and you're only using one instance of it. Also, the code is importingmatplotlib.pyplotbut then not directly using it since the plotting is handled by the pandas backend.-import sys -import sys -import os +import os +import sys sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), "../..")))🧰 Tools
🪛 Ruff (0.8.2)
13-13: Redefinition of unused
sysfrom line 12Remove definition:
sys(F811)
pyopenms_viz/_core.py (9)
17-19: Remove duplicate imports or consolidate them.There are duplicate imports of matplotlib.pyplot, math.ceil, and warnings in multiple places in the file. These should be consolidated at the top of the file.
-import matplotlib.pyplot as plt -from math import ceil -These imports already exist elsewhere in the file (lines 544-546 and 717-724) and are duplicative.
544-546: Remove duplicate imports.These imports are duplicated from earlier in the file (lines 17-19 and lines 20, 39).
-from math import ceil -import matplotlib.pyplot as plt -import warnings🧰 Tools
🪛 Ruff (0.8.2)
544-544: Redefinition of unused
ceilfrom line 20(F811)
545-545: Redefinition of unused
pltfrom line 17Remove definition:
plt(F811)
546-546: Redefinition of unused
warningsfrom line 39Remove definition:
warnings(F811)
588-606: Add a stacklevel parameter to warnings.When issuing warnings, include a stacklevel parameter to help users identify where the warning originates from.
warnings.warn( f"tile_by column '{tile_by}' not found in data. Plot will be generated without tiling." + , stacklevel=2 ) warnings.warn("tile_columns must be a positive integer. Defaulting to 1." + , stacklevel=2 )Also, consider simplifying the nested if statement for tile_columns validation:
-if hasattr(self._config, "tile_columns"): - if not isinstance(self._config.tile_columns, int) or self._config.tile_columns < 1: +if hasattr(self._config, "tile_columns") and (not isinstance(self._config.tile_columns, int) or self._config.tile_columns < 1):🧰 Tools
🪛 Ruff (0.8.2)
596-596: No explicit
stacklevelkeyword argument found(B028)
602-603: Use a single
ifstatement instead of nestedifstatements(SIM102)
604-604: No explicit
stacklevelkeyword argument found(B028)
607-674: Well-implemented tiling functionality, but tooltips could be refactored.The implementation for automatic subplot creation based on the
tile_byparameter is well-structured and handles both grouped and non-grouped cases appropriately. However, the tooltip creation logic is duplicated in both the tiling case and the fallback case.Consider extracting the tooltip creation logic into a helper method to reduce code duplication:
def _create_chromatogram_tooltips(self, data): """Create tooltips for chromatogram data""" tooltip_entries = {"retention time": self.x, "intensity": self.y} if "Annotation" in data.columns: tooltip_entries["annotation"] = "Annotation" if "product_mz" in data.columns: tooltip_entries["product m/z"] = "product_mz" return self._create_tooltips(tooltip_entries, index=False)Then you could simplify both the grouped and non-grouped sections:
# In the grouped section: tooltips, custom_hover_data = self._create_chromatogram_tooltips(group_df) # In the fallback section: tooltips, custom_hover_data = self._create_chromatogram_tooltips(self.data)
717-725: Remove redundant imports.These imports are duplicate declarations of modules already imported earlier in the file.
-import warnings -from math import ceil -from typing import List, Literal -import re -import numpy as np -from pandas import DataFrame, concat -from numpy import nan -from statistics import mean -# Import your other necessary modules (e.g., for ColorGenerator, mz_tolerance_binning, sturges_rule, freedman_diaconis_rule)🧰 Tools
🪛 Ruff (0.8.2)
717-717: Redefinition of unused
warningsfrom line 546Remove definition:
warnings(F811)
718-718: Redefinition of unused
ceilfrom line 544Remove definition:
ceil(F811)
719-719: Redefinition of unused
Listfrom line 4Remove definition:
List(F811)
719-719: Redefinition of unused
Literalfrom line 4Remove definition:
Literal(F811)
720-720: Redefinition of unused
refrom line 15Remove definition:
re(F811)
721-721: Redefinition of unused
npfrom line 5Remove definition:
np(F811)
722-722: Redefinition of unused
DataFramefrom line 11(F811)
722-722: Redefinition of unused
concatfrom line 10Remove definition:
concat(F811)
723-723: Redefinition of unused
nanfrom line 20Remove definition:
nan(F811)
724-724: Redefinition of unused
meanfrom line 20(F811)
751-778: Implement validation consistently across classes.The
SpectrumPlotclass duplicates thetile_byvalidation logic that was moved to a dedicated method inChromatogramPlot. For consistency, you should follow the same pattern and extract this logic to a_validate_plot_configmethod.def __init__(self, data, **kwargs) -> None: super().__init__(data, **kwargs) # (Other validations like _check_and_aggregate_duplicates, sorting, etc.) self._check_and_aggregate_duplicates() # Sort data by x (and by self.by if provided) if self.by is not None: self.data.sort_values(by=[self.by, self.x], inplace=True) else: self.data.sort_values(by=self.x, inplace=True) # Convert to relative intensity if required. if self.relative_intensity: self.data[self.y] = self.data[self.y] / self.data[self.y].max() * 100 - # Validate tiling configuration in the constructor - if hasattr(self._config, "tile_by"): - tile_by = self._config.tile_by - if tile_by not in self.data.columns: - warnings.warn( - f"tile_by column '{tile_by}' not found in data. Plot will be generated without tiling." - ) - self._config.tile_by = None - # (Other configuration validations can be added here as needed.) + # Validate plot configuration + self._validate_plot_config() # Proceed to generate the plot self.plot() +def _validate_plot_config(self): + """ + Validate plot configuration options (e.g., tiling parameters) before plotting. + """ + # Validate the tile_by option: check if the specified column exists in the data. + if hasattr(self._config, "tile_by"): + tile_by = self._config.tile_by + if tile_by not in self.data.columns: + warnings.warn( + f"tile_by column '{tile_by}' not found in data. Plot will be generated without tiling.", + stacklevel=2 + ) + self._config.tile_by = None + + # Validate tile_columns: ensure it is a positive integer. + if hasattr(self._config, "tile_columns") and (not isinstance(self._config.tile_columns, int) or self._config.tile_columns < 1): + warnings.warn("tile_columns must be a positive integer. Defaulting to 1.", stacklevel=2) + self._config.tile_columns = 1🧰 Tools
🪛 Ruff (0.8.2)
771-771: No explicit
stacklevelkeyword argument found(B028)
839-904: Apply the same refactoring pattern to SpectrumPlot.Similar to the suggestion for
ChromatogramPlot, theSpectrumPlotimplementation could benefit from extracting repetitive logic into helper methods to reduce duplication between the tiled and non-tiled modes.Consider creating helper methods for:
- Preparing tooltips for spectrum data
- Setting up and configuring the line renderer
- Handling mirror spectrum logic
This would make the code more maintainable and easier to understand.
975-981: Fix multi-statement lines for better readability.Multiple statements on one line make the code harder to read. Each statement should be on its own line.
- cols = [self.x] - if self.by is not None: cols.append(self.by) - if self.peak_color is not None: cols.append(self.peak_color) - if self.ion_annotation is not None: cols.append(self.ion_annotation) - if self.sequence_annotation is not None: cols.append(self.sequence_annotation) - if self.custom_annotation is not None: cols.append(self.custom_annotation) - if self.annotation_color is not None: cols.append(self.annotation_color) + cols = [self.x] + if self.by is not None: + cols.append(self.by) + if self.peak_color is not None: + cols.append(self.peak_color) + if self.ion_annotation is not None: + cols.append(self.ion_annotation) + if self.sequence_annotation is not None: + cols.append(self.sequence_annotation) + if self.custom_annotation is not None: + cols.append(self.custom_annotation) + if self.annotation_color is not None: + cols.append(self.annotation_color)🧰 Tools
🪛 Ruff (0.8.2)
975-975: Multiple statements on one line (colon)
(E701)
976-976: Multiple statements on one line (colon)
(E701)
977-977: Multiple statements on one line (colon)
(E701)
978-978: Multiple statements on one line (colon)
(E701)
979-979: Multiple statements on one line (colon)
(E701)
980-980: Multiple statements on one line (colon)
(E701)
998-998: Use Python's boolean semantics properly.Avoid comparing directly to
TrueorFalse. Use Python's truthiness checks instead.- if self.bin_peaks == True or self.bin_peaks == "auto": + if self.bin_peaks or self.bin_peaks == "auto":🧰 Tools
🪛 Ruff (0.8.2)
998-998: Avoid equality comparisons to
True; useif self.bin_peaks:for truth checksReplace with
self.bin_peaks(E712)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/gallery_scripts_template/plot_investigate_spectrum_binning_ms_matplotlib.py(2 hunks)docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py(2 hunks)pyopenms_viz/__init__.py(1 hunks)pyopenms_viz/_config.py(1 hunks)pyopenms_viz/_core.py(9 hunks)
✅ Files skipped from review due to trivial changes (1)
- pyopenms_viz/init.py
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/gallery_scripts_template/plot_investigate_spectrum_binning_ms_matplotlib.py
- pyopenms_viz/_config.py
🧰 Additional context used
🧬 Code Definitions (2)
pyopenms_viz/_core.py (2)
pyopenms_viz/_config.py (1)
SpectrumConfig(340-397)pyopenms_viz/_misc.py (2)
ColorGenerator(10-127)Colors(25-37)
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py (1)
pyopenms_viz/_core.py (5)
plot(346-350)plot(607-674)plot(712-714)plot(836-952)plot(1175-1187)
🪛 Ruff (0.8.2)
pyopenms_viz/_core.py
544-544: Redefinition of unused ceil from line 20
(F811)
545-545: Redefinition of unused plt from line 17
Remove definition: plt
(F811)
546-546: Redefinition of unused warnings from line 39
Remove definition: warnings
(F811)
596-596: No explicit stacklevel keyword argument found
(B028)
602-603: Use a single if statement instead of nested if statements
(SIM102)
604-604: No explicit stacklevel keyword argument found
(B028)
717-717: Redefinition of unused warnings from line 546
Remove definition: warnings
(F811)
718-718: Redefinition of unused ceil from line 544
Remove definition: ceil
(F811)
719-719: Redefinition of unused List from line 4
Remove definition: List
(F811)
719-719: Redefinition of unused Literal from line 4
Remove definition: Literal
(F811)
720-720: Redefinition of unused re from line 15
Remove definition: re
(F811)
721-721: Redefinition of unused np from line 5
Remove definition: np
(F811)
722-722: Redefinition of unused DataFrame from line 11
(F811)
722-722: Redefinition of unused concat from line 10
Remove definition: concat
(F811)
723-723: Redefinition of unused nan from line 20
Remove definition: nan
(F811)
724-724: Redefinition of unused mean from line 20
(F811)
771-771: No explicit stacklevel keyword argument found
(B028)
975-975: Multiple statements on one line (colon)
(E701)
976-976: Multiple statements on one line (colon)
(E701)
977-977: Multiple statements on one line (colon)
(E701)
978-978: Multiple statements on one line (colon)
(E701)
979-979: Multiple statements on one line (colon)
(E701)
980-980: Multiple statements on one line (colon)
(E701)
998-998: Avoid equality comparisons to True; use if self.bin_peaks: for truth checks
Replace with self.bin_peaks
(E712)
1034-1034: Remove unnecessary True if ... else False
Remove unnecessary True if ... else False
(SIM210)
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py
11-11: matplotlib.pyplot imported but unused
Remove unused import: matplotlib.pyplot
(F401)
13-13: Redefinition of unused sys from line 12
Remove definition: sys
(F811)
🔇 Additional comments (7)
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py (4)
28-36: LGTM! Clean consistent error handling.The implementation maintains appropriate error handling for the network request.
40-45: LGTM! Consistent error handling for zip extraction.The implementation correctly catches
BadZipFileexceptions when extracting the zip file.
47-50: LGTM! Clear data loading.The implementation loads data from the downloaded files in a straightforward way.
51-73: Great improvements to plotting interface using tile_by!The new implementation leverages the
tile_byparameter to automatically create subplots based on therun_namecolumn, significantly simplifying the code compared to manually iterating through run names and creating individual subplots.The main advantages of this approach:
- Eliminates the need for manual subplot creation and management
- Reduces code duplication
- Makes the interface more intuitive and user-friendly
- Provides a consistent layout through the
tile_columnsparameterThis is an excellent example of API improvement that simplifies the user experience while maintaining full functionality.
pyopenms_viz/_core.py (3)
572-581: LGTM! Improved data preparation logic.The code properly sorts data by the x-axis (and by group if provided) and handles relative intensity conversion correctly.
582-586: Good separation of validation logic.Separating the validation logic into a dedicated method improves code organization and maintainability.
676-680: LGTM! Improved method documentation.The updated docstring is clearer and more concise.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
pyopenms_viz/_core.py (1)
642-642: 🛠️ Refactor suggestionUse modify_y_range function instead of direct matplotlib calls.
Direct manipulations of matplotlib objects should be abstracted away using the existing
_modify_y_rangefunction.- ax.set_ylim(0, group_df[self.y].max()) + self._modify_y_range((0, group_df[self.y].max()), (0, 0.1), canvas=ax)This ensures that backend-specific operations are properly abstracted.
🧹 Nitpick comments (7)
pyopenms_viz/_core.py (7)
584-602: Improve the validation method with proper warning stacklevels and simplified conditionals.The validation method is a good addition, but there are some improvements that can be made to follow best practices.
Consider these improvements:
- Add
stacklevel=2to thewarnings.warn()calls to ensure warnings point to the caller- Simplify the nested if statements
def _validate_plot_config(self): """ Validate plot configuration options (e.g., tiling parameters) before plotting. """ # Validate the tile_by option: check if the specified column exists in the data. if hasattr(self._config, "tile_by") and self._config.tile_by is not None: tile_by = self._config.tile_by if tile_by not in self.data.columns: warnings.warn( f"tile_by column '{tile_by}' not found in data. Plot will be generated without tiling.", stacklevel=2 ) self._config.tile_by = None # Validate tile_columns: ensure it is a positive integer. if hasattr(self._config, "tile_columns") and self._config.tile_columns is not None: if not isinstance(self._config.tile_columns, int) or self._config.tile_columns < 1: warnings.warn("tile_columns must be a positive integer. Defaulting to 1.", stacklevel=2) self._config.tile_columns = 1🧰 Tools
🪛 Ruff (0.8.2)
592-592: No explicit
stacklevelkeyword argument found(B028)
598-599: Use a single
ifstatement instead of nestedifstatements(SIM102)
600-600: No explicit
stacklevelkeyword argument found(B028)
627-632: Extract tooltip creation from the loop.As noted in a previous review comment, the tooltips code can be moved out of the if-else
tile_byloop to avoid duplication.# Create tooltips once before the tile_by check tooltip_entries = {"retention time": self.x, "intensity": self.y} if "Annotation" in self.data.columns: tooltip_entries["annotation"] = "Annotation" if "product_mz" in self.data.columns: tooltip_entries["product m/z"] = "product_mz" tooltips, custom_hover_data = self._create_tooltips(tooltip_entries, index=False) if tile_by: # Grouping and subplot creation # ... for i, (group_val, group_df) in enumerate(grouped): # Use group-specific data with the pre-created tooltip format # ...
635-641: Pass canvas and title directly to the get_line_renderer method.As noted in previous review comments,
canvasandtitleparameters should be passed directly to theget_line_renderermethod.- linePlot = self.get_line_renderer(data=group_df, config=self._config) - linePlot.canvas = ax - linePlot.generate(tooltips, custom_hover_data) - ax.set_title(f"{tile_by}: {group_val}", fontsize=14) + linePlot = self.get_line_renderer( + data=group_df, + config=self._config, + canvas=ax, + title=f"{tile_by}: {group_val}" + ) + linePlot.generate(tooltips, custom_hover_data)
667-670: Move interactive boundary drawing outside of the if-else block.As noted in previous review comments, this code can be moved out of the if-else loop to avoid duplication.
# After both branches of the if-else block if self._interactive: self.manual_boundary_renderer = self._add_bounding_vertical_drawer() if self.annotation_data is not None: self._add_peak_boundaries(self.annotation_data)
713-721: Remove redundant imports.These imports are duplicating imports that already exist earlier in the file. This can cause confusion and subtle bugs.
-import warnings -from math import ceil -from typing import List, Literal -import re -import numpy as np -from pandas import DataFrame, concat -from numpy import nan -from statistics import mean -# Import your other necessary modules (e.g., for ColorGenerator, mz_tolerance_binning, sturges_rule, freedman_diaconis_rule)These modules are already imported at the top of the file (lines 4-39).
🧰 Tools
🪛 Ruff (0.8.2)
713-713: Redefinition of unused
warningsfrom line 39Remove definition:
warnings(F811)
714-714: Redefinition of unused
ceilfrom line 20(F811)
715-715: Redefinition of unused
Listfrom line 4Remove definition:
List(F811)
715-715: Redefinition of unused
Literalfrom line 4Remove definition:
Literal(F811)
716-716: Redefinition of unused
refrom line 15Remove definition:
re(F811)
717-717: Redefinition of unused
npfrom line 5Remove definition:
np(F811)
718-718: Redefinition of unused
DataFramefrom line 11(F811)
718-718: Redefinition of unused
concatfrom line 10Remove definition:
concat(F811)
719-719: Redefinition of unused
nanfrom line 20Remove definition:
nan(F811)
720-720: Redefinition of unused
meanfrom line 20(F811)
971-977: Fix style issues with multiple statements on a single line.The code has multiple statements on single lines, which violates PEP 8 style guidelines.
- if self.by is not None: cols.append(self.by) - if self.peak_color is not None: cols.append(self.peak_color) - if self.ion_annotation is not None: cols.append(self.ion_annotation) - if self.sequence_annotation is not None: cols.append(self.sequence_annotation) - if self.custom_annotation is not None: cols.append(self.custom_annotation) - if self.annotation_color is not None: cols.append(self.annotation_color) + if self.by is not None: + cols.append(self.by) + if self.peak_color is not None: + cols.append(self.peak_color) + if self.ion_annotation is not None: + cols.append(self.ion_annotation) + if self.sequence_annotation is not None: + cols.append(self.sequence_annotation) + if self.custom_annotation is not None: + cols.append(self.custom_annotation) + if self.annotation_color is not None: + cols.append(self.annotation_color)🧰 Tools
🪛 Ruff (0.8.2)
971-971: Multiple statements on one line (colon)
(E701)
972-972: Multiple statements on one line (colon)
(E701)
973-973: Multiple statements on one line (colon)
(E701)
974-974: Multiple statements on one line (colon)
(E701)
975-975: Multiple statements on one line (colon)
(E701)
976-976: Multiple statements on one line (colon)
(E701)
994-994: Fix boolean comparison style.It's more idiomatic in Python to use the truthiness of a variable directly rather than comparing to True.
- if self.bin_peaks == True or self.bin_peaks == "auto": + if self.bin_peaks or self.bin_peaks == "auto":🧰 Tools
🪛 Ruff (0.8.2)
994-994: Avoid equality comparisons to
True; useif self.bin_peaks:for truth checksReplace with
self.bin_peaks(E712)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pyopenms_viz/_core.py(8 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
pyopenms_viz/_core.py (5)
pyopenms_viz/_bokeh/core.py (6)
plot(292-320)plot(329-385)plot(402-438)fig(53-54)fig(57-59)generate(229-242)pyopenms_viz/_matplotlib/core.py (7)
plot(294-316)plot(325-385)plot(405-469)plot(606-623)ax(40-41)ax(44-46)generate(231-241)pyopenms_viz/_plotly/core.py (6)
plot(284-306)plot(312-461)plot(475-528)fig(40-41)fig(44-46)generate(129-142)pyopenms_viz/_config.py (1)
SpectrumConfig(340-397)pyopenms_viz/_misc.py (2)
ColorGenerator(10-127)Colors(25-37)
🪛 Ruff (0.8.2)
pyopenms_viz/_core.py
592-592: No explicit stacklevel keyword argument found
(B028)
598-599: Use a single if statement instead of nested if statements
(SIM102)
600-600: No explicit stacklevel keyword argument found
(B028)
713-713: Redefinition of unused warnings from line 39
Remove definition: warnings
(F811)
714-714: Redefinition of unused ceil from line 20
(F811)
715-715: Redefinition of unused List from line 4
Remove definition: List
(F811)
715-715: Redefinition of unused Literal from line 4
Remove definition: Literal
(F811)
716-716: Redefinition of unused re from line 15
Remove definition: re
(F811)
717-717: Redefinition of unused np from line 5
Remove definition: np
(F811)
718-718: Redefinition of unused DataFrame from line 11
(F811)
718-718: Redefinition of unused concat from line 10
Remove definition: concat
(F811)
719-719: Redefinition of unused nan from line 20
Remove definition: nan
(F811)
720-720: Redefinition of unused mean from line 20
(F811)
767-767: No explicit stacklevel keyword argument found
(B028)
971-971: Multiple statements on one line (colon)
(E701)
972-972: Multiple statements on one line (colon)
(E701)
973-973: Multiple statements on one line (colon)
(E701)
974-974: Multiple statements on one line (colon)
(E701)
975-975: Multiple statements on one line (colon)
(E701)
976-976: Multiple statements on one line (colon)
(E701)
994-994: Avoid equality comparisons to True; use if self.bin_peaks: for truth checks
Replace with self.bin_peaks
(E712)
1030-1030: Remove unnecessary True if ... else False
Remove unnecessary True if ... else False
(SIM210)
🔇 Additional comments (1)
pyopenms_viz/_core.py (1)
578-580: Good addition of configuration validation.Adding validation for the plotting configuration before proceeding with the actual plotting is a good practice that will help catch configuration errors early.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
pyopenms_viz/_core.py (1)
17-19:⚠️ Potential issueBackend-specific imports in core module.
Importing matplotlib directly in the core module violates the architecture's design principle. This module should remain backend-agnostic, with backend-specific code confined to their respective implementation files (e.g.,
_matplotlib/core.py).-import matplotlib.pyplot as plt -from math import ceilThese imports should be moved to the appropriate backend implementation file. The core module should only define abstract interfaces that are implemented by each backend.
🧹 Nitpick comments (4)
pyopenms_viz/_core.py (4)
584-602: The validation logic is good but missing stacklevel parameter in warnings.The validation logic is well-structured but the
warnings.warncalls are missing thestacklevelparameter, which helps in pointing to the correct location in the call stack where the warning originated.if tile_by not in self.data.columns: warnings.warn( - f"tile_by column '{tile_by}' not found in data. Plot will be generated without tiling." + f"tile_by column '{tile_by}' not found in data. Plot will be generated without tiling.", + stacklevel=2 ) self._config.tile_by = None # Validate tile_columns: ensure it is a positive integer. if hasattr(self._config, "tile_columns"): if not isinstance(self._config.tile_columns, int) or self._config.tile_columns < 1: warnings.warn("tile_columns must be a positive integer. Defaulting to 1.") + stacklevel=2 self._config.tile_columns = 1🧰 Tools
🪛 Ruff (0.8.2)
592-592: No explicit
stacklevelkeyword argument found(B028)
598-599: Use a single
ifstatement instead of nestedifstatements(SIM102)
600-600: No explicit
stacklevelkeyword argument found(B028)
961-967: Improve code formatting for better readability.Multiple statements on a single line (using colons) make the code harder to read and maintain.
-if self.by is not None: cols.append(self.by) -if self.peak_color is not None: cols.append(self.peak_color) -if self.ion_annotation is not None: cols.append(self.ion_annotation) -if self.sequence_annotation is not None: cols.append(self.sequence_annotation) -if self.custom_annotation is not None: cols.append(self.custom_annotation) -if self.annotation_color is not None: cols.append(self.annotation_color) +if self.by is not None: + cols.append(self.by) +if self.peak_color is not None: + cols.append(self.peak_color) +if self.ion_annotation is not None: + cols.append(self.ion_annotation) +if self.sequence_annotation is not None: + cols.append(self.sequence_annotation) +if self.custom_annotation is not None: + cols.append(self.custom_annotation) +if self.annotation_color is not None: + cols.append(self.annotation_color)🧰 Tools
🪛 Ruff (0.8.2)
961-961: Multiple statements on one line (colon)
(E701)
962-962: Multiple statements on one line (colon)
(E701)
963-963: Multiple statements on one line (colon)
(E701)
964-964: Multiple statements on one line (colon)
(E701)
965-965: Multiple statements on one line (colon)
(E701)
966-966: Multiple statements on one line (colon)
(E701)
984-984: Use boolean directly in condition check.Comparing directly to
Trueis unnecessary and less pythonic.-if self.bin_peaks == True or self.bin_peaks == "auto": +if self.bin_peaks or self.bin_peaks == "auto":🧰 Tools
🪛 Ruff (0.8.2)
984-984: Avoid equality comparisons to
True; useif self.bin_peaks:for truth checksReplace with
self.bin_peaks(E712)
1020-1020: Simplify the conditional expression.The conditional expression can be simplified for better readability.
-data = data.sort_values(y, ascending=True if data[y].min() < 0 else False).reset_index() +data = data.sort_values(y, ascending=data[y].min() < 0).reset_index()🧰 Tools
🪛 Ruff (0.8.2)
1020-1020: Remove unnecessary
True if ... else FalseRemove unnecessary
True if ... else False(SIM210)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pyopenms_viz/_core.py(7 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
pyopenms_viz/_core.py (2)
pyopenms_viz/_config.py (1)
SpectrumConfig(340-397)pyopenms_viz/_misc.py (2)
ColorGenerator(10-127)Colors(25-37)
🪛 Ruff (0.8.2)
pyopenms_viz/_core.py
592-592: No explicit stacklevel keyword argument found
(B028)
598-599: Use a single if statement instead of nested if statements
(SIM102)
600-600: No explicit stacklevel keyword argument found
(B028)
757-757: No explicit stacklevel keyword argument found
(B028)
961-961: Multiple statements on one line (colon)
(E701)
962-962: Multiple statements on one line (colon)
(E701)
963-963: Multiple statements on one line (colon)
(E701)
964-964: Multiple statements on one line (colon)
(E701)
965-965: Multiple statements on one line (colon)
(E701)
966-966: Multiple statements on one line (colon)
(E701)
984-984: Avoid equality comparisons to True; use if self.bin_peaks: for truth checks
Replace with self.bin_peaks
(E712)
1020-1020: Remove unnecessary True if ... else False
Remove unnecessary True if ... else False
(SIM210)
🔇 Additional comments (2)
pyopenms_viz/_core.py (2)
578-580: LGTM - Adding validation for plotting configuration is a good improvement.Adding a dedicated validation method for the plot configuration ensures robust error handling before attempting to render the plot.
673-676: LGTM - Docstring simplification is appropriate.The simplified docstring provides clear information about the method's purpose and clarifies that it can be overridden when needed.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pyopenms_viz/_core.py (1)
17-19:⚠️ Potential issueRemove backend-specific imports from the core module.
The core module should remain backend-agnostic, but these imports make it matplotlib-dependent. This violates the architecture principle of separating the core logic from specific backend implementations.
-import matplotlib.pyplot as plt -from math import ceilThese imports should be moved to the appropriate backend-specific files (e.g.,
_matplotlib/core.py).🧰 Tools
🪛 Ruff (0.8.2)
17-17:
matplotlib.pyplotimported but unusedRemove unused import:
matplotlib.pyplot(F401)
🧹 Nitpick comments (7)
pyopenms_viz/_core.py (6)
584-602: Enhance validation with stacklevel in warnings.The validation logic is well-structured but missing stacklevel parameter in warnings, which helps users see the correct source file in warnings.
warnings.warn( f"tile_by column '{tile_by}' not found in data. Plot will be generated without tiling.", + stacklevel=2 ) warnings.warn("tile_columns must be a positive integer. Defaulting to 1.", + stacklevel=2 )🧰 Tools
🪛 Ruff (0.8.2)
592-592: No explicit
stacklevelkeyword argument found(B028)
598-599: Use a single
ifstatement instead of nestedifstatements(SIM102)
600-600: No explicit
stacklevelkeyword argument found(B028)
752-758: Follow the same pattern for tile validation across plot types.The validation for the
tile_byparameter is implemented slightly differently in SpectrumPlot compared to ChromatogramPlot. This could lead to inconsistent behavior.Extract common validation logic into a shared method:
def _validate_tile_by(self, column_name): """Validate that a tile_by column exists in the data.""" if column_name not in self.data.columns: warnings.warn( f"tile_by column '{column_name}' not found in data. Plot will be generated without tiling.", stacklevel=2 ) return None return column_name🧰 Tools
🪛 Ruff (0.8.2)
755-755: No explicit
stacklevelkeyword argument found(B028)
957-962: Fix multi-statement lines for improved readability.Multiple statements on single lines reduce code readability and violate Python style conventions.
- if self.by is not None: cols.append(self.by) - if self.peak_color is not None: cols.append(self.peak_color) - if self.ion_annotation is not None: cols.append(self.ion_annotation) - if self.sequence_annotation is not None: cols.append(self.sequence_annotation) - if self.custom_annotation is not None: cols.append(self.custom_annotation) - if self.annotation_color is not None: cols.append(self.annotation_color) + if self.by is not None: + cols.append(self.by) + if self.peak_color is not None: + cols.append(self.peak_color) + if self.ion_annotation is not None: + cols.append(self.ion_annotation) + if self.sequence_annotation is not None: + cols.append(self.sequence_annotation) + if self.custom_annotation is not None: + cols.append(self.custom_annotation) + if self.annotation_color is not None: + cols.append(self.annotation_color)🧰 Tools
🪛 Ruff (0.8.2)
957-957: Multiple statements on one line (colon)
(E701)
958-958: Multiple statements on one line (colon)
(E701)
959-959: Multiple statements on one line (colon)
(E701)
960-960: Multiple statements on one line (colon)
(E701)
961-961: Multiple statements on one line (colon)
(E701)
962-962: Multiple statements on one line (colon)
(E701)
980-980: Simplify boolean condition.Using equality comparisons with
Trueis unnecessarily verbose.- if self.bin_peaks == True or self.bin_peaks == "auto": + if self.bin_peaks or self.bin_peaks == "auto":🧰 Tools
🪛 Ruff (0.8.2)
980-980: Avoid equality comparisons to
True; useif self.bin_peaks:for truth checksReplace with
self.bin_peaks(E712)
1016-1016: Simplify conditional expression.The conditional expression is using an unnecessary ternary operator.
- data = data.sort_values(y, ascending=True if data[y].min() < 0 else False).reset_index() + data = data.sort_values(y, ascending=data[y].min() < 0).reset_index()🧰 Tools
🪛 Ruff (0.8.2)
1016-1016: Remove unnecessary
True if ... else FalseRemove unnecessary
True if ... else False(SIM210)
1087-1088: Consider adding a _compute_y_range_and_padding helper for SpectrumPlot.The _compute_y_range_and_padding method added to the MATPLOTLIBPlot class would be useful for all spectrum plots to ensure consistent y-axis ranges and padding.
I see that a similar calculation is made in line 932-933 for spectrum plots. Consider adding a similar helper to BaseMSPlot to keep logic centralized.
pyopenms_viz/_matplotlib/core.py (1)
60-74: Good abstraction of subplot creation.This method properly encapsulates the subplot creation logic, making it reusable across different plotting scenarios. However, the docstring mentions a parameter that's not in the method signature.
""" Create a grid of subplots using matplotlib. Args: rows (int): Number of subplot rows. columns (int): Number of subplot columns. - figsize (Tuple[float, float]): Size of the figure in inches. Returns: Tuple[Figure, List[Axes]]: The matplotlib Figure and a flattened list of Axes. """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/gallery_scripts_template/plot_investigate_spectrum_binning_ms_matplotlib.py(2 hunks)pyopenms_viz/_core.py(8 hunks)pyopenms_viz/_matplotlib/core.py(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
pyopenms_viz/_matplotlib/core.py (3)
pyopenms_viz/_plotly/core.py (2)
fig(40-41)fig(44-46)pyopenms_viz/_bokeh/core.py (2)
fig(53-54)fig(57-59)pyopenms_viz/_core.py (2)
canvas(125-126)canvas(129-130)
pyopenms_viz/_core.py (4)
pyopenms_viz/_matplotlib/core.py (9)
plot(345-367)plot(376-436)plot(456-520)plot(657-674)_create_subplots(60-74)ax(91-92)ax(95-97)generate(282-292)_modify_y_range(248-264)pyopenms_viz/_plotly/core.py (7)
plot(284-306)plot(312-461)plot(475-528)fig(40-41)fig(44-46)generate(129-142)_modify_y_range(226-235)pyopenms_viz/_config.py (1)
SpectrumConfig(340-397)pyopenms_viz/_misc.py (2)
ColorGenerator(10-127)Colors(25-37)
🪛 Ruff (0.8.2)
pyopenms_viz/_core.py
17-17: matplotlib.pyplot imported but unused
Remove unused import: matplotlib.pyplot
(F401)
592-592: No explicit stacklevel keyword argument found
(B028)
598-599: Use a single if statement instead of nested if statements
(SIM102)
600-600: No explicit stacklevel keyword argument found
(B028)
755-755: No explicit stacklevel keyword argument found
(B028)
957-957: Multiple statements on one line (colon)
(E701)
958-958: Multiple statements on one line (colon)
(E701)
959-959: Multiple statements on one line (colon)
(E701)
960-960: Multiple statements on one line (colon)
(E701)
961-961: Multiple statements on one line (colon)
(E701)
962-962: Multiple statements on one line (colon)
(E701)
980-980: Avoid equality comparisons to True; use if self.bin_peaks: for truth checks
Replace with self.bin_peaks
(E712)
1016-1016: Remove unnecessary True if ... else False
Remove unnecessary True if ... else False
(SIM210)
🔇 Additional comments (5)
pyopenms_viz/_matplotlib/core.py (2)
38-57: Clean implementation of y-range helper.The y-range and padding computation is now properly abstracted as a helper method. This improves code organization and reusability.
77-87: Proper axes cleanup abstraction.The method effectively handles the removal of unused subplot axes, which is essential for maintaining a clean visualization when the number of data groups doesn't match the grid size.
docs/gallery_scripts_template/plot_investigate_spectrum_binning_ms_matplotlib.py (3)
27-36: Good demonstration of the new tiling functionality.This example effectively demonstrates how to use the new tiling feature by creating a 'Run' column and duplicating the dataset for multiple runs. This shows users how to group their data for tiled visualizations.
95-104: Enhanced plot configuration with the new parameters.The update to include
by="Run"andshow_plot=Falseshowcases how to use the new tiling functionality in practice. This helps users understand how to integrate these features into their own workflows.
111-111: Corrected display method.Using
plt.show()instead offig.show()follows Matplotlib's standard practices and ensures proper display of the figure.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
pyopenms_viz/_core.py (5)
583-600: Fix warnings.warn() calls to include stacklevel parameter.The
warnings.warn()calls should include thestacklevelparameter to ensure warnings point to the caller's code rather than the warning generator.- warnings.warn( - f"tile_by column '{tile_by}' not found in data. Plot will be generated without tiling." - ) + warnings.warn( + f"tile_by column '{tile_by}' not found in data. Plot will be generated without tiling.", + stacklevel=2 + )Also, consider simplifying the nested if statement structure:
- if hasattr(self._config, "tile_columns"): - if not isinstance(self._config.tile_columns, int) or self._config.tile_columns < 1: + if hasattr(self._config, "tile_columns") and (not isinstance(self._config.tile_columns, int) or self._config.tile_columns < 1):🧰 Tools
🪛 Ruff (0.8.2)
590-590: No explicit
stacklevelkeyword argument found(B028)
596-597: Use a single
ifstatement instead of nestedifstatements(SIM102)
598-598: No explicit
stacklevelkeyword argument found(B028)
955-962: Improve readability by avoiding multiple statements on a single line.Multiple if statements on single lines reduce code readability and make maintenance more difficult.
- if self.by is not None: cols.append(self.by) - if self.peak_color is not None: cols.append(self.peak_color) - if self.ion_annotation is not None: cols.append(self.ion_annotation) - if self.sequence_annotation is not None: cols.append(self.sequence_annotation) - if self.custom_annotation is not None: cols.append(self.custom_annotation) - if self.annotation_color is not None: cols.append(self.annotation_color) + if self.by is not None: + cols.append(self.by) + if self.peak_color is not None: + cols.append(self.peak_color) + if self.ion_annotation is not None: + cols.append(self.ion_annotation) + if self.sequence_annotation is not None: + cols.append(self.sequence_annotation) + if self.custom_annotation is not None: + cols.append(self.custom_annotation) + if self.annotation_color is not None: + cols.append(self.annotation_color)🧰 Tools
🪛 Ruff (0.8.2)
955-955: Multiple statements on one line (colon)
(E701)
956-956: Multiple statements on one line (colon)
(E701)
957-957: Multiple statements on one line (colon)
(E701)
958-958: Multiple statements on one line (colon)
(E701)
959-959: Multiple statements on one line (colon)
(E701)
960-960: Multiple statements on one line (colon)
(E701)
978-980: Use idiomatic boolean checks.Using
== Trueis less idiomatic than a direct boolean check in Python.- if self.bin_peaks == True or self.bin_peaks == "auto": + if self.bin_peaks is True or self.bin_peaks == "auto":Even better:
- if self.bin_peaks == True or self.bin_peaks == "auto": + if self.bin_peaks or self.bin_peaks == "auto":🧰 Tools
🪛 Ruff (0.8.2)
978-978: Avoid equality comparisons to
True; useif self.bin_peaks:for truth checksReplace with
self.bin_peaks(E712)
1014-1014: Simplify unnecessary ternary expression.The ternary expression can be simplified to use the direct boolean value.
- data = data.sort_values(y, ascending=True if data[y].min() < 0 else False).reset_index() + data = data.sort_values(y, ascending=data[y].min() < 0).reset_index()🧰 Tools
🪛 Ruff (0.8.2)
1014-1014: Remove unnecessary
True if ... else FalseRemove unnecessary
True if ... else False(SIM210)
930-931: Add a helper method for y-range and padding computation.This appears to be a new pattern that computes y-range and padding for visualization. This should be extracted into a helper method to avoid duplication.
Add a helper method that centralizes this computation:
def _compute_y_range_and_padding(self, spectrum, reference_spectrum=None): """ Compute the y-axis range and padding based on spectrum data. Args: spectrum: The primary spectrum data reference_spectrum: Optional reference spectrum data Returns: Tuple of (y_range, y_padding) """ # Calculate range based on the data if reference_spectrum is not None: y_range = (min(spectrum[self.y].min(), reference_spectrum[self.y].min()), max(spectrum[self.y].max(), reference_spectrum[self.y].max())) else: y_range = (spectrum[self.y].min(), spectrum[self.y].max()) # Calculate appropriate padding y_padding = (0.05, 0.1) # Adjust these values as needed return y_range, y_paddingThis would make the code more maintainable and centralize the logic for consistent behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pyopenms_viz/_core.py(8 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
pyopenms_viz/_core.py (2)
pyopenms_viz/_config.py (1)
SpectrumConfig(340-397)pyopenms_viz/_misc.py (2)
ColorGenerator(10-127)Colors(25-37)
🪛 Ruff (0.8.2)
pyopenms_viz/_core.py
590-590: No explicit stacklevel keyword argument found
(B028)
596-597: Use a single if statement instead of nested if statements
(SIM102)
598-598: No explicit stacklevel keyword argument found
(B028)
753-753: No explicit stacklevel keyword argument found
(B028)
955-955: Multiple statements on one line (colon)
(E701)
956-956: Multiple statements on one line (colon)
(E701)
957-957: Multiple statements on one line (colon)
(E701)
958-958: Multiple statements on one line (colon)
(E701)
959-959: Multiple statements on one line (colon)
(E701)
960-960: Multiple statements on one line (colon)
(E701)
978-978: Avoid equality comparisons to True; use if self.bin_peaks: for truth checks
Replace with self.bin_peaks
(E712)
1014-1014: Remove unnecessary True if ... else False
Remove unnecessary True if ... else False
(SIM210)
🔇 Additional comments (1)
pyopenms_viz/_core.py (1)
576-578: Good addition of validation method.The addition of a dedicated validation method for the plot configuration is a good practice that helps ensure the data and parameters are valid before plotting.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py (1)
11-11: Remove unused import.The
matplotlib.pyplotimport is not used in this script as the plotting is now handled directly through the pandas API with the custom backend.-import matplotlib.pyplot as plt🧰 Tools
🪛 Ruff (0.8.2)
11-11:
matplotlib.pyplotimported but unusedRemove unused import:
matplotlib.pyplot(F401)
pyopenms_viz/_core.py (8)
576-600: Add stacklevel to warning calls for better traceability.The warnings should include a
stacklevelparameter to ensure the warning points to the correct location in the user's code rather than the library internals.- warnings.warn( - f"facet_column column '{facet_column}' not found in data. Plot will be generated without tiling." - ) + warnings.warn( + f"facet_column column '{facet_column}' not found in data. Plot will be generated without tiling.", + stacklevel=2 + ) - warnings.warn("facet_col_wrap must be a positive integer. Defaulting to 1.") + warnings.warn("facet_col_wrap must be a positive integer. Defaulting to 1.", stacklevel=2)🧰 Tools
🪛 Ruff (0.8.2)
590-590: No explicit
stacklevelkeyword argument found(B028)
596-597: Use a single
ifstatement instead of nestedifstatements(SIM102)
598-598: No explicit
stacklevelkeyword argument found(B028)
596-600: Simplify nested conditional.The nested if statement can be simplified for better readability.
- if hasattr(self._config, "facet_col_wrap"): - if not isinstance(self._config.facet_col_wrap, int) or self._config.facet_col_wrap < 1: - warnings.warn("facet_col_wrap must be a positive integer. Defaulting to 1.") - self._config.facet_col_wrap = 1 + if hasattr(self._config, "facet_col_wrap") and (not isinstance(self._config.facet_col_wrap, int) or self._config.facet_col_wrap < 1): + warnings.warn("facet_col_wrap must be a positive integer. Defaulting to 1.", stacklevel=2) + self._config.facet_col_wrap = 1🧰 Tools
🪛 Ruff (0.8.2)
596-597: Use a single
ifstatement instead of nestedifstatements(SIM102)
598-598: No explicit
stacklevelkeyword argument found(B028)
615-638: Extract common tiling logic to reduce duplication.There's significant code duplication between the tiling implementations in ChromatogramPlot and SpectrumPlot. This violates the DRY principle and increases maintenance burden.
Extract the common tiling functionality to a shared method or base class:
# Add to BaseMSPlot or create a new TileMixin class def _handle_tiling(self, data, facet_column, plot_function): """ Handle common tiling functionality across plot types. Args: data: DataFrame to be plotted facet_column: Column to group by for tiling plot_function: Function that handles plotting for each tile Returns: The figure with subplots """ if not facet_column: return None # Group the data and calculate grid dimensions grouped = data.groupby(facet_column) num_groups = len(grouped) facet_col_wrap = self._config.facet_col_wrap tile_rows = int(ceil(num_groups / facet_col_wrap)) # Create figure and axes grid fig, axes = self._create_subplots(tile_rows, facet_col_wrap) # Plot each group for i, (group_val, group_df) in enumerate(grouped): if i < len(axes): ax = axes[i] title = f"{facet_column}: {group_val}" plot_function(group_df, ax, title, i) # Remove unused axes self._delete_extra_axes(axes, i+1) return fig
749-757: Add stacklevel to warning call in SpectrumPlot.Similar to the earlier issue, this warning should include a stacklevel parameter.
- warnings.warn( - f"facet_column column '{facet_column}' not found in data. Plot will be generated without tiling." - ) + warnings.warn( + f"facet_column column '{facet_column}' not found in data. Plot will be generated without tiling.", + stacklevel=2 + )🧰 Tools
🪛 Ruff (0.8.2)
753-753: No explicit
stacklevelkeyword argument found(B028)
955-961: Fix multiple statements on one line.The code has multiple statements on a single line which reduces readability. Each statement should be on its own line.
- cols = [self.x] - if self.by is not None: cols.append(self.by) - if self.peak_color is not None: cols.append(self.peak_color) - if self.ion_annotation is not None: cols.append(self.ion_annotation) - if self.sequence_annotation is not None: cols.append(self.sequence_annotation) - if self.custom_annotation is not None: cols.append(self.custom_annotation) - if self.annotation_color is not None: cols.append(self.annotation_color) + cols = [self.x] + if self.by is not None: + cols.append(self.by) + if self.peak_color is not None: + cols.append(self.peak_color) + if self.ion_annotation is not None: + cols.append(self.ion_annotation) + if self.sequence_annotation is not None: + cols.append(self.sequence_annotation) + if self.custom_annotation is not None: + cols.append(self.custom_annotation) + if self.annotation_color is not None: + cols.append(self.annotation_color)🧰 Tools
🪛 Ruff (0.8.2)
955-955: Multiple statements on one line (colon)
(E701)
956-956: Multiple statements on one line (colon)
(E701)
957-957: Multiple statements on one line (colon)
(E701)
958-958: Multiple statements on one line (colon)
(E701)
959-959: Multiple statements on one line (colon)
(E701)
960-960: Multiple statements on one line (colon)
(E701)
978-978: Fix boolean comparison style.Avoid direct equality comparison with
True; use the boolean value directly for truth checks.- if self.bin_peaks == True or self.bin_peaks == "auto": + if self.bin_peaks or self.bin_peaks == "auto":🧰 Tools
🪛 Ruff (0.8.2)
978-978: Avoid equality comparisons to
True; useif self.bin_peaks:for truth checksReplace with
self.bin_peaks(E712)
1014-1014: Simplify conditional expression.The ternary expression can be simplified to directly use the boolean result.
- data = data.sort_values(y, ascending=True if data[y].min() < 0 else False).reset_index() + data = data.sort_values(y, ascending=data[y].min() < 0).reset_index()🧰 Tools
🪛 Ruff (0.8.2)
1014-1014: Remove unnecessary
True if ... else FalseRemove unnecessary
True if ... else False(SIM210)
732-757: Centralize validation logic.The validation logic for facet_column is duplicated between ChromatogramPlot and SpectrumPlot. This violates the DRY principle and could lead to inconsistent behavior.
Extract this common validation into a shared method in a base class:
# Add to BaseMSPlot def _validate_facet_column(self): """Validate that the facet_column exists in the data.""" if hasattr(self._config, "facet_column"): facet_column = self._config.facet_column if facet_column is not None and facet_column not in self.data.columns: warnings.warn( f"facet_column column '{facet_column}' not found in data. Plot will be generated without tiling.", stacklevel=2 ) self._config.facet_column = None return self._config.facet_columnThen call this method from both plot classes.
🧰 Tools
🪛 Ruff (0.8.2)
753-753: No explicit
stacklevelkeyword argument found(B028)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py(2 hunks)pyopenms_viz/_config.py(1 hunks)pyopenms_viz/_core.py(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pyopenms_viz/_config.py
🧰 Additional context used
🧬 Code Definitions (1)
pyopenms_viz/_core.py (4)
pyopenms_viz/_bokeh/core.py (6)
plot(292-320)plot(329-385)plot(402-438)fig(53-54)fig(57-59)generate(229-242)pyopenms_viz/_matplotlib/core.py (8)
plot(345-367)plot(376-436)plot(456-520)plot(657-674)_create_subplots(60-74)ax(91-92)ax(95-97)generate(282-292)pyopenms_viz/_config.py (1)
SpectrumConfig(340-397)pyopenms_viz/_misc.py (1)
ColorGenerator(10-127)
🪛 Ruff (0.8.2)
pyopenms_viz/_core.py
590-590: No explicit stacklevel keyword argument found
(B028)
596-597: Use a single if statement instead of nested if statements
(SIM102)
598-598: No explicit stacklevel keyword argument found
(B028)
753-753: No explicit stacklevel keyword argument found
(B028)
955-955: Multiple statements on one line (colon)
(E701)
956-956: Multiple statements on one line (colon)
(E701)
957-957: Multiple statements on one line (colon)
(E701)
958-958: Multiple statements on one line (colon)
(E701)
959-959: Multiple statements on one line (colon)
(E701)
960-960: Multiple statements on one line (colon)
(E701)
978-978: Avoid equality comparisons to True; use if self.bin_peaks: for truth checks
Replace with self.bin_peaks
(E712)
1014-1014: Remove unnecessary True if ... else False
Remove unnecessary True if ... else False
(SIM210)
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py
11-11: matplotlib.pyplot imported but unused
Remove unused import: matplotlib.pyplot
(F401)
13-13: Redefinition of unused sys from line 12
Remove definition: sys
(F811)
🔇 Additional comments (3)
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py (1)
51-73: LGTM! Simplified plotting interface is well implemented.The refactored code demonstrates the enhanced plotting interface using the new
facet_columnparameter, which automatically groups data byrun_nameand creates subplots in a single column layout. This is a significant improvement over manually creating and iterating through subplots.pyopenms_viz/_core.py (2)
649-649: Verify if tight_layout is necessary.According to a past review comment, the
tight_layout()call might be redundant as it should be called later in the process.Please verify if this
tight_layout()call is necessary or if it's already handled elsewhere in the rendering pipeline. If it's handled elsewhere, consider removing this duplicate call.
663-667: LGTM! Simplified docstring.The docstring for
_add_peak_boundarieshas been simplified to indicate that it should be overridden by subclasses, which is a good practice for abstract or partial methods.
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py (2)
11-14: Unused import detected:matplotlib.pyplotThe
matplotlib.pyplotmodule is imported but never used directly in the code. The plotting is handled through pandas with thems_matplotlibbackend.import pandas as pd import requests import zipfile -import matplotlib.pyplot as plt import sys import os sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), "../..")))🧰 Tools
🪛 Ruff (0.8.2)
11-11:
matplotlib.pyplotimported but unusedRemove unused import:
matplotlib.pyplot(F401)
16-17: Consider removing unnecessary blank linesThere are two consecutive blank lines here where one would be sufficient.
sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), "../.."))) - # Set the plotting backend
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py
11-11: matplotlib.pyplot imported but unused
Remove unused import: matplotlib.pyplot
(F401)
🔇 Additional comments (2)
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py (2)
47-48: LGTM: Well-structured data loadingThe data loading code is clear and appropriately uses pandas to read the TSV files.
50-72: Great improvement using the new faceting functionalityThis is a significant improvement over manually creating subplots. Using the new
facet_columnparameter automates the subplot creation process and makes the code much more concise and maintainable. The documentation comments clearly explain the change from the previous approach.One small suggestion - consider adding a brief comment about what the
facet_col_wrap=1parameter actually does, as it might not be immediately obvious to all users:facet_column="run_name", # Automatically groups data by run_name and creates subplots - facet_col_wrap=1, # Layout: 1 column (one subplot per row) + facet_col_wrap=1, # Creates a single column layout (stacks subplots vertically)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pyopenms_viz/_matplotlib/core.py (1)
62-77: Docstring and code discrepancy onfigsize.The docstring mentions a
figsizeparameter, but the function neither accepts nor uses it. Consider removing the mention offigsizefrom the docstring or updating the function to accept and apply afigsizeparameter if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pyopenms_viz/_matplotlib/core.py(2 hunks)test/conftest.py(1 hunks)test/test_chromatogram.py(1 hunks)test/test_spectrum.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- test/test_chromatogram.py
- test/test_spectrum.py
🧰 Additional context used
🧬 Code Definitions (1)
pyopenms_viz/_matplotlib/core.py (3)
pyopenms_viz/_plotly/core.py (2)
fig(40-41)fig(44-46)pyopenms_viz/_bokeh/core.py (2)
fig(53-54)fig(57-59)pyopenms_viz/_core.py (2)
canvas(123-124)canvas(127-128)
🔇 Additional comments (3)
test/conftest.py (1)
3-5: LGTM: Proper path configuration for test imports.Adding the parent directory to
sys.pathis a common practice for enabling imports from the parent module in test files. This change ensures that the tests can properly import the package being tested without requiring installation, which is particularly useful during development.pyopenms_viz/_matplotlib/core.py (2)
7-8: Use of non-interactive backend seems appropriate.Setting the backend to "Agg" is typically used to render plots in a non-interactive, headless environment (e.g., within CI/CD). This looks fine as long as there’s no requirement for interactive plots.
40-58: Validate handling of negative or empty data scenarios.Currently, the function forces
min_valueto 0 when noreference_spectrumis provided. Ifspectrum[self.y]might contain negative intensities or be empty, this logic may be insufficient or produce incorrect plot ranges. Consider verifying whether 0 is the intended lower bound or if additional checks are needed for empty or negative values.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pyopenms_viz/_matplotlib/core.py (1)
62-76: Docstring mentions unused parameterfigsize.The docstring references a
figsizeparameter that doesn't exist in the method signature. This could confuse future developers.- def _create_subplots(self, rows: int, columns: int): + def _create_subplots(self, rows: int, columns: int):- Args: - rows (int): Number of subplot rows. - columns (int): Number of subplot columns. - figsize (Tuple[float, float]): Size of the figure in inches. + Args: + rows (int): Number of subplot rows. + columns (int): Number of subplot columns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pyopenms_viz/_matplotlib/core.py(2 hunks)
🔇 Additional comments (3)
pyopenms_viz/_matplotlib/core.py (3)
7-8: Good choice for setting the "Agg" backend.Setting the Matplotlib backend to "Agg" is a good practice for non-interactive environments, ensuring consistent rendering across different platforms and headless environments.
40-58: LGTM: Well-structured helper method for computing y-axis ranges.The method effectively handles both cases - with and without reference spectrum. The comment "Ensure this is a tuple" on line 57 confirms your attention to detail in maintaining consistent return types.
79-89: Great fix: Using fig.delaxes instead of canvas.delaxes.This implementation correctly addresses the previously identified issue by using
self.fig.delaxes()instead ofself.canvas.delaxes(), ensuring proper removal of extra axes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/conftest.py (1)
3-5: Remove unused imports or uncomment usage codeThe
osandsysmodules are imported but never used since the line that would use them (line 5) is commented out. Either remove these unused imports or uncomment the system path modification if it's needed.-import os -import sys -# sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), "..")))🧰 Tools
🪛 Ruff (0.8.2)
3-3:
osimported but unusedRemove unused import:
os(F401)
4-4:
sysimported but unusedRemove unused import:
sys(F401)
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py (1)
11-14: Remove unused importsThe
matplotlib.pyplot,sys, andosmodules are imported but never used in the code. The path appending line is commented out, making these imports unnecessary.import pandas as pd import requests import zipfile -import matplotlib.pyplot as plt -import sys -import os -# sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), "../..")))🧰 Tools
🪛 Ruff (0.8.2)
11-11:
matplotlib.pyplotimported but unusedRemove unused import:
matplotlib.pyplot(F401)
12-12:
sysimported but unusedRemove unused import:
sys(F401)
13-13:
osimported but unusedRemove unused import:
os(F401)
docs/gallery_scripts_template/plot_investigate_spectrum_binning_ms_matplotlib.py (1)
12-16: Remove unused imports and commented codeThe
sysandosmodules are imported but never used since the path appending line is commented out.import pandas as pd import matplotlib.pyplot as plt import requests from io import StringIO -import sys -import os -# # Add parent directories to the path (adjust as necessary) -# sys.path.append(os.path.abspath(os.path.join(os.path.dirname(__file__), "../..")))🧰 Tools
🪛 Ruff (0.8.2)
12-12:
sysimported but unusedRemove unused import:
sys(F401)
13-13:
osimported but unusedRemove unused import:
os(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/gallery_scripts_template/plot_investigate_spectrum_binning_ms_matplotlib.py(2 hunks)docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py(2 hunks)test/conftest.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py
11-11: matplotlib.pyplot imported but unused
Remove unused import: matplotlib.pyplot
(F401)
12-12: sys imported but unused
Remove unused import: sys
(F401)
13-13: os imported but unused
Remove unused import: os
(F401)
docs/gallery_scripts_template/plot_investigate_spectrum_binning_ms_matplotlib.py
12-12: sys imported but unused
Remove unused import: sys
(F401)
13-13: os imported but unused
Remove unused import: os
(F401)
test/conftest.py
3-3: os imported but unused
Remove unused import: os
(F401)
4-4: sys imported but unused
Remove unused import: sys
(F401)
🔇 Additional comments (4)
docs/gallery_scripts_template/plot_spyogenes_subplots_ms_matplotlib.py (1)
51-72: Great implementation of the new tile_by functionalityThe code has been refactored to use the new
facet_columnparameter which simplifies subplot creation. This aligns with the PR objective of simplifying the MultiPlot interface and removing the need for manual canvas specification.The improvements make the code more concise and easier to understand, while providing the same functionality. The
facet_column="run_name"parameter automatically groups data by run_name and creates the necessary subplots.docs/gallery_scripts_template/plot_investigate_spectrum_binning_ms_matplotlib.py (3)
27-35: Great implementation of run grouping for multi-plot functionalityThis code adds a 'Run' column and duplicates entries for each run group, which is an excellent way to demonstrate the new tile_by functionality. The approach of creating a list of DataFrames and then concatenating them is clean and maintainable.
89-103: Effective demonstration of the facet capabilityThe code has been updated to leverage the new
by="Run"parameter which automatically creates subplots grouped by the Run column. The addition ofshow_plot=Falseprevents immediate display, which is appropriate when creating multiple plots for a figure.
111-111: Switch from fig.show() to plt.show() is appropriateChanging from
fig.show()toplt.show()is the standard way to display figures in matplotlib and ensures consistent behavior across different environments.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pyopenms_viz/_bokeh/core.py (1)
563-563: Remove redundant pass statementThe
passstatement is now redundant since the class has an implementation of theplotmethod. This could confuse readers about whether the method is implemented or not.- pass
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pyopenms_viz/_bokeh/core.py(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
pyopenms_viz/_bokeh/core.py (2)
pyopenms_viz/_matplotlib/core.py (6)
_compute_y_range_and_padding(40-58)plot(348-370)plot(379-439)plot(459-523)plot(660-677)_modify_y_range(251-267)pyopenms_viz/_core.py (6)
plot(344-348)plot(601-661)plot(704-706)plot(819-931)plot(1155-1167)_modify_y_range(374-386)
|
Hi @jcharkow I have solved most of the conflicts if you want you can review it. |
I'll wait untill all the conflicts are solved and then I'll review |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
pyopenms_viz/_core.py (6)
577-600: Good implementation of validation for facet configuration.The validation logic checks that both
facet_columnexists in the data and thatfacet_col_wrapis a positive integer, providing appropriate warnings when these conditions aren't met.However, you should add the
stacklevelparameter to your warning calls:warnings.warn( f"facet_column column '{facet_column}' not found in data. Plot will be generated without tiling.", + stacklevel=2 )warnings.warn("facet_col_wrap must be a positive integer. Defaulting to 1.", + stacklevel=2 )🧰 Tools
🪛 Ruff (0.8.2)
590-590: No explicit
stacklevelkeyword argument found(B028)
596-597: Use a single
ifstatement instead of nestedifstatements(SIM102)
598-598: No explicit
stacklevelkeyword argument found(B028)
749-757: Duplicated validation in SpectrumPlot.init.This validation logic is duplicated from the ChromatogramPlot class. Consider abstracting this to a common parent class or helper method to avoid code duplication.
# In BaseMSPlot or a new mixin class def _validate_facet_column(self, column_name): """Validate that a facet_column exists in the data.""" if column_name is not None and column_name not in self.data.columns: warnings.warn( f"facet_column column '{column_name}' not found in data. Plot will be generated without tiling.", stacklevel=2 ) return None return column_nameAlso, add the
stacklevelparameter to the warning:warnings.warn( f"facet_column column '{facet_column}' not found in data. Plot will be generated without tiling.", + stacklevel=2 )🧰 Tools
🪛 Ruff (0.8.2)
753-753: No explicit
stacklevelkeyword argument found(B028)
823-928: Well-structured tiling implementation for SpectrumPlot.The implementation handles tiling appropriately, with good attention to detail for spectrum-specific features like reference spectra and mirror plotting.
However, there's significant code duplication between this implementation and the ChromatogramPlot version. Consider extracting common tiling logic to a shared method in the BaseMSPlot class.
954-961: Improve code style in _bin_peaks method.The current implementation has multiple statements on the same line with semicolons, which reduces readability and goes against common Python style guidelines.
- if self.by is not None: cols.append(self.by) - if self.peak_color is not None: cols.append(self.peak_color) - if self.ion_annotation is not None: cols.append(self.ion_annotation) - if self.sequence_annotation is not None: cols.append(self.sequence_annotation) - if self.custom_annotation is not None: cols.append(self.custom_annotation) - if self.annotation_color is not None: cols.append(self.annotation_color) + if self.by is not None: + cols.append(self.by) + if self.peak_color is not None: + cols.append(self.peak_color) + if self.ion_annotation is not None: + cols.append(self.ion_annotation) + if self.sequence_annotation is not None: + cols.append(self.sequence_annotation) + if self.custom_annotation is not None: + cols.append(self.custom_annotation) + if self.annotation_color is not None: + cols.append(self.annotation_color)🧰 Tools
🪛 Ruff (0.8.2)
955-955: Multiple statements on one line (colon)
(E701)
956-956: Multiple statements on one line (colon)
(E701)
957-957: Multiple statements on one line (colon)
(E701)
958-958: Multiple statements on one line (colon)
(E701)
959-959: Multiple statements on one line (colon)
(E701)
960-960: Multiple statements on one line (colon)
(E701)
978-978: Clean up comparison with boolean literal.Use Python's truthiness checking instead of explicit comparison with True.
- if self.bin_peaks == True or self.bin_peaks == "auto": + if self.bin_peaks or self.bin_peaks == "auto":🧰 Tools
🪛 Ruff (0.8.2)
978-978: Avoid equality comparisons to
True; useif self.bin_peaks:for truth checksReplace with
self.bin_peaks(E712)
1014-1014: Simplify boolean expression in _get_annotations.The current ternary expression
True if data[y].min() < 0 else Falsecan be simplified.- data = data.sort_values(y, ascending=True if data[y].min() < 0 else False).reset_index() + data = data.sort_values(y, ascending=data[y].min() < 0).reset_index()🧰 Tools
🪛 Ruff (0.8.2)
1014-1014: Remove unnecessary
True if ... else FalseRemove unnecessary
True if ... else False(SIM210)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pyopenms_viz/_core.py(8 hunks)pyopenms_viz/_matplotlib/core.py(2 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
pyopenms_viz/_matplotlib/core.py (2)
pyopenms_viz/_plotly/core.py (2)
fig(40-41)fig(44-46)pyopenms_viz/_bokeh/core.py (2)
fig(53-54)fig(57-59)
pyopenms_viz/_core.py (5)
pyopenms_viz/_bokeh/core.py (6)
plot(292-320)plot(329-385)plot(402-438)fig(53-54)fig(57-59)generate(229-242)pyopenms_viz/_matplotlib/core.py (7)
plot(339-361)plot(370-430)plot(450-514)plot(651-668)ax(85-86)ax(89-91)generate(276-286)pyopenms_viz/_plotly/core.py (6)
plot(284-306)plot(312-461)plot(475-528)fig(40-41)fig(44-46)generate(129-142)pyopenms_viz/_config.py (1)
SpectrumConfig(340-397)pyopenms_viz/_misc.py (1)
ColorGenerator(10-127)
🪛 Ruff (0.8.2)
pyopenms_viz/_core.py
590-590: No explicit stacklevel keyword argument found
(B028)
596-597: Use a single if statement instead of nested if statements
(SIM102)
598-598: No explicit stacklevel keyword argument found
(B028)
753-753: No explicit stacklevel keyword argument found
(B028)
955-955: Multiple statements on one line (colon)
(E701)
956-956: Multiple statements on one line (colon)
(E701)
957-957: Multiple statements on one line (colon)
(E701)
958-958: Multiple statements on one line (colon)
(E701)
959-959: Multiple statements on one line (colon)
(E701)
960-960: Multiple statements on one line (colon)
(E701)
978-978: Avoid equality comparisons to True; use if self.bin_peaks: for truth checks
Replace with self.bin_peaks
(E712)
1014-1014: Remove unnecessary True if ... else False
Remove unnecessary True if ... else False
(SIM210)
🔇 Additional comments (9)
pyopenms_viz/_core.py (5)
606-613: Looks good - clear tooltip generation logic.The tooltip preparation is well-organized, including comprehensive data fields that users would want to see.
615-640: Well-structured tiling implementation for ChromatogramPlot.The implementation correctly groups data by
facet_column, calculates the grid dimensions, and creates subplots. The approach manages axis assignment and titling appropriately.
647-648: Good abstraction of axes cleanup.Moving the extra axes handling to a dedicated method improves maintainability and follows the recommendations from previous reviews.
652-661: Clean fallback for single-plot case.The logic cleanly handles the case when no tiling is requested, falling back to the standard single plot implementation.
930-931: Good utilization of helper method for y-range calculation.Using the
_compute_y_range_and_paddinghelper keeps the code cleaner and more maintainable. This is a good practice that helps reduce duplication and centralize range calculation logic.pyopenms_viz/_matplotlib/core.py (4)
7-8: Appropriate backend specification for non-interactive plotting.Setting the matplotlib backend to "Agg" is good practice for ensuring consistent rendering, especially in non-interactive environments like CI/CD pipelines or server-side generation.
40-49: Well-implemented y-range calculation helper method.The
_compute_y_range_and_paddingmethod centralizes the logic for calculating y-axis ranges and padding, which is used in the core module. This is a good example of abstracting backend-specific implementations.
53-67: Good implementation of subplot creation.The
_create_subplotsmethod provides a clean implementation for creating a grid of matplotlib subplots, with good documentation and appropriate return values.
70-80:❓ Verification inconclusive
Bug in _delete_extra_axes method.
The method tries to access
self.fig.delaxes, but it's not clear whereself.figis defined or set in this context. This might cause runtime errors.- self.fig.delaxes(axes[j]) + plt.gcf().delaxes(axes[j])Or ensure that
self.figis correctly set before this method is called.
🏁 Script executed:
#!/bin/bash # Check if the fig attribute is consistently set in the MATPLOTLIBPlot class grep -n "self.fig" pyopenms_viz/_matplotlib/core.py | grep -v "delaxes"Length of output: 503
Attention: Verify that
_delete_extra_axesis called only afterself.figis initializedI reviewed the usage of
self.figin the module—it's clearly set later in the file (e.g., lines 115, 122, and 645). This indicates that, under normal usage,self.fig.delaxes(axes[j])should work fine. However, please ensure that the_delete_extra_axesmethod is never invoked beforeself.figis initialized.
- If there’s any chance that
_delete_extra_axesmight run too early, consider:
- Reordering the initialization so that
self.figis set before this method can be called, or- Using a fallback (e.g.,
plt.gcf().delaxes(axes[j])) to safeguard against uninitialized state.
2 participants
feat(ChromatogramPlot): Add tile_by parameter for automated subplot creation
tile_byparameter in ChromatogramPlot to split data into subplots automatically.tile_bycolumn exists in both the chromatogram and annotation data.Summary by CodeRabbit