fix/refactor: minor code clean up and fix for annotations in peakmaps#98
fix/refactor: minor code clean up and fix for annotations in peakmaps#98singjc merged 6 commits intoOpenMS:mainfrom
Conversation
* pandas peakmap plots failed in matplotlib backend because MarkerShapeGenerator defaulted to an uninitialized cycle and next(self.marker) raised on None. * Persist engine/n size hints and lazily bootstrap the cycle, so defaults auto-initialize and existing backends keep their behavior. * Adds a defensive engine validator and preserves explicit shape lists, preventing regressions for plotly/bokeh marker cycling.
…actoring param config logic.
…iple custom lines and labels
…ns in MATPLOTLIBPeakMapPlot
WalkthroughThis pull request refactors annotation and legend configuration across the visualization library. It introduces new configuration fields for annotation styling (colormap, line width/type) and legend display, adds helper methods to the base configuration class, and updates plotting backends to use the new annotation-based configuration properties instead of feature-based ones. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pyopenms_viz/_bokeh/core.py (1)
644-663: Fix annotation color handling in box boundariesWhen an annotation color column is supplied, the rectangle still gets colored with
next(color_gen), so user-defined colors are ignored. Even without a color column, the extranext(color_gen)call advances the iterator twice per feature and desynchronizes the palette. Please reuse the resolvedcolorvariable for the glyph stroke.if self.annotation_colors in feature: - color = feature[self.annotation_colors] + color = feature[self.annotation_colors] else: color = next(color_gen) @@ - box_boundary_lines = self.fig.rect( + box_boundary_lines = self.fig.rect( x=center_x, y=center_y, width=width, height=height, - color=next(color_gen), + line_color=color, line_dash=self.annotation_line_type, line_width=self.annotation_line_width, fill_alpha=0, )pyopenms_viz/_matplotlib/core.py (1)
567-568: Critical: Method defined in wrong scope.The
get_manual_bounding_box_coordsmethod is incorrectly defined inside the_add_peak_boundariesmethod body. It should be defined at the class level as an instance method ofMATPLOTLIBChromatogramPlot.Apply this diff to fix the indentation:
- # since matplotlib is not interactive cannot implement the following methods - def get_manual_bounding_box_coords(self): - pass + # since matplotlib is not interactive cannot implement the following methods + def get_manual_bounding_box_coords(self): + passNote: This should be dedented by 4 spaces to align with other class methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pyopenms_viz/_bokeh/core.py(2 hunks)pyopenms_viz/_config.py(5 hunks)pyopenms_viz/_core.py(14 hunks)pyopenms_viz/_matplotlib/core.py(5 hunks)pyopenms_viz/_misc.py(5 hunks)pyopenms_viz/_plotly/core.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
pyopenms_viz/_plotly/core.py (2)
pyopenms_viz/_config.py (1)
bokeh_line_dash_mapper(524-571)pyopenms_viz/_core.py (9)
BaseMSPlot(493-544)BasePlot(115-431)ChromatogramPlot(547-631)LinePlot(434-441)MobilogramPlot(634-653)PeakMapPlot(1117-1348)ScatterPlot(473-490)SpectrumPlot(656-1114)VLinePlot(444-470)
pyopenms_viz/_bokeh/core.py (1)
pyopenms_viz/_core.py (8)
BaseMSPlot(493-544)BasePlot(115-431)ChromatogramPlot(547-631)LinePlot(434-441)PeakMapPlot(1117-1348)ScatterPlot(473-490)SpectrumPlot(656-1114)show(401-407)
pyopenms_viz/_core.py (2)
pyopenms_viz/_config.py (1)
BasePlotConfig(134-238)pyopenms_viz/__init__.py (1)
_get_plot_backend(193-202)
pyopenms_viz/_config.py (1)
pyopenms_viz/_misc.py (2)
ColorGenerator(11-128)MarkerShapeGenerator(131-279)
pyopenms_viz/_matplotlib/core.py (3)
pyopenms_viz/_config.py (2)
LegendConfig(62-130)_matplotlibLegendLocationMapper(94-110)pyopenms_viz/_core.py (3)
BaseMSPlot(493-544)SpectrumPlot(656-1114)show(401-407)pyopenms_viz/_misc.py (1)
ColorGenerator(11-128)
🪛 Ruff (0.14.2)
pyopenms_viz/_misc.py
251-251: Avoid specifying long messages outside the exception class
(TRY003)
pyopenms_viz/_core.py
131-131: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: test (windows-latest, 3.12)
🔇 Additional comments (5)
pyopenms_viz/_matplotlib/core.py (5)
5-27: LGTM!The import reorganization improves code maintainability by alphabetizing imports and adding necessary imports (Rectangle) for the box boundary drawing functionality.
60-60: LGTM!Appropriate change from f-string to plain string since there are no variables to interpolate.
502-564: LGTM!The refactoring to use annotation-based configuration properties (
annotation_colormap,annotation_line_width,annotation_line_type,annotation_legend_config) is consistent with the PR objectives and improves the clarity of the annotation rendering logic.
728-790: LGTM!The refactoring consistently applies annotation-based configuration throughout the method. The variable naming is clear (
legend_itemsfor labels,custom_lines_listfor Rectangle handles), and the legend construction properly uses the newncolfield fromannotation_legend_configto control column layout.
793-794: LGTM!The method stub is correctly defined at the class level with proper indentation, unlike the one in
MATPLOTLIBChromatogramPlot.
| "annotation_color" | ||
| if self.annotation_color is None | ||
| else self.annotation_color | ||
| ) |
There was a problem hiding this comment.
Drop stray stdout debug print
print(f"Annotation color: ...") will fire on every spectrum plot and spam stdout. Please remove this debug statement (or replace it with proper logging if needed).
if self.annotation_color is None:
data["annotation_color"] = "black"
- print(f"Annotation color: {self.annotation_color}")
annotation_color_column = (
"annotation_color"
if self.annotation_color is None🤖 Prompt for AI Agents
In pyopenms_viz/_core.py around lines 996 to 1003 there is a stray debug print
that prints the annotation color to stdout on every spectrum plot; remove the
print(f"Annotation color: {self.annotation_color}") line (or replace it with a
proper logger.debug(...) call using the module logger) and ensure behavior
otherwise remains the same (keep the annotation_color defaulting and
column-selection logic intact).
1 participant
This pull request contains a series of code cleanups, refactorings, ruff formating, and some bug fixes. Some of the params for controlling annotation params were not added/removed with the refactoring of how configs are handled.
Enhancements to annotation configuration:
PeakMapConfigand its factory for annotation customization, includingannotation_colormap,annotation_line_width,annotation_line_type, andannotation_legend_config, as well as additional fields for column names and color mapping. These changes allow for more flexible and detailed annotation rendering in plots. [1] [2]__post_init__method inPeakMapConfigto ensure thatannotation_legend_configis always aLegendConfiginstance, improving type safety and consistency.Code organization and cleanup:
pyopenms_viz/_bokeh/core.py,pyopenms_viz/_matplotlib/core.py, andpyopenms_viz/_core.pyfor better readability and maintainability. This includes deduplication, alphabetization, and removal of unused imports. [1] [2] [3] [4]API and doc improvements:
pyopenms_viz/_core.pyto single-line return statements for conciseness and consistency. [1] [2] [3] [4] [5]Bugfixes and logic improvements:
_get_annotationsmethod to clarify assignment of the annotation color column and ensure correct handling of annotation color defaults. [1] [2]Miscellaneous:
ncolfield toLegendConfigfor specifying the number of columns in legends, improving legend layout control.Summary by CodeRabbit
New Features
Bug Fixes
Refactor