Fix: Plotly Spectrum Annotation bug - #139
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThe PR updates the testing infrastructure to support syrupy 5.0.0+ by migrating snapshot extensions to binary serialization mode, fixes a debug print statement in annotation handling, improves tooltip data alignment in Plotly traces, and regenerates snapshot test files with updated Bokeh version references and binary-encoded data payloads. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Pull request overview
This pull request fixes an issue where annotations do not appear correctly when plotting a single trace spectrum in the Plotly backend. The issue occurs because when spectrum data is converted to line plot format (to draw stems), each m/z value is repeated 3 times, but the custom hover data wasn't being expanded to match this increased trace length for single-trace scenarios.
Changes:
- Added logic to expand
custom_hover_datawhen a single trace has more points than the original data (e.g., when spectrum plotting repeats each m/z value multiple times to draw stems) - Removed a debug print statement that was left in the code
- Imported the
repeatfunction from NumPy to support the data expansion
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pyopenms_viz/_plotly/core.py | Added logic in _add_tooltips to handle single-trace scenarios where the trace length is expanded by repeating data points, and imported the repeat function from NumPy |
| pyopenms_viz/_core.py | Removed a debug print statement for annotation color |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
🤖 Fix all issues with AI agents
In `@pyopenms_viz/_plotly/core.py`:
- Around line 163-180: The code can raise ZeroDivisionError when
custom_hover_data has zero rows; in the block that computes ratio =
int(trace_len / custom_hover_data.shape[0]) (used to possibly repeat
custom_hover_data via repeat) add a defensive guard to ensure
custom_hover_data.shape[0] > 0 before performing the division and only
compute/repeat when that condition holds (otherwise skip expansion or leave
custom_hover_data as-is), then call self.fig.update_traces(...) with the
unchanged custom_hover_data.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pyopenms_viz/_core.pypyopenms_viz/_plotly/core.py
⏰ 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). (3)
- GitHub Check: CodeQL analysis (python)
- GitHub Check: Agent
- GitHub Check: build
🔇 Additional comments (2)
pyopenms_viz/_core.py (1)
996-998: LGTM! Debug print statement removal.Removing stray debug output is good hygiene before merging.
pyopenms_viz/_plotly/core.py (1)
7-7: LGTM!The
repeatimport is necessary for the new hover data alignment logic.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| else: | ||
| # If there is a single trace but the trace x-array was expanded | ||
| # (for example spectrum plotting repeats each mz value several times | ||
| # to draw stems), expand the custom_hover_data to match the trace length. | ||
| trace = self.fig.data[0] | ||
| trace_len = len(trace.x) if hasattr(trace, "x") else None | ||
| if ( | ||
| trace_len is not None | ||
| and custom_hover_data is not None | ||
| and custom_hover_data.shape[0] != trace_len | ||
| ): | ||
| # If trace length is an integer multiple of custom data rows, | ||
| # repeat each row accordingly. | ||
| ratio = int(trace_len / custom_hover_data.shape[0]) | ||
| if ratio > 1 and custom_hover_data.shape[0] * ratio == trace_len: | ||
| custom_hover_data = repeat(custom_hover_data, ratio, axis=0) | ||
|
|
||
| self.fig.update_traces(hovertemplate=tooltips, customdata=custom_hover_data) |
There was a problem hiding this comment.
Guard against division by zero when custom_hover_data is empty.
The logic correctly aligns hover data for single-trace spectra where x-values are repeated (e.g., for stem plots). However, if custom_hover_data has zero rows, line 176 will raise a ZeroDivisionError.
🛡️ Suggested defensive guard
else:
# If there is a single trace but the trace x-array was expanded
# (for example spectrum plotting repeats each mz value several times
# to draw stems), expand the custom_hover_data to match the trace length.
trace = self.fig.data[0]
trace_len = len(trace.x) if hasattr(trace, "x") else None
if (
trace_len is not None
and custom_hover_data is not None
+ and custom_hover_data.shape[0] > 0
and custom_hover_data.shape[0] != trace_len
):
# If trace length is an integer multiple of custom data rows,
# repeat each row accordingly.
ratio = int(trace_len / custom_hover_data.shape[0])
if ratio > 1 and custom_hover_data.shape[0] * ratio == trace_len:
custom_hover_data = repeat(custom_hover_data, ratio, axis=0)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| else: | |
| # If there is a single trace but the trace x-array was expanded | |
| # (for example spectrum plotting repeats each mz value several times | |
| # to draw stems), expand the custom_hover_data to match the trace length. | |
| trace = self.fig.data[0] | |
| trace_len = len(trace.x) if hasattr(trace, "x") else None | |
| if ( | |
| trace_len is not None | |
| and custom_hover_data is not None | |
| and custom_hover_data.shape[0] != trace_len | |
| ): | |
| # If trace length is an integer multiple of custom data rows, | |
| # repeat each row accordingly. | |
| ratio = int(trace_len / custom_hover_data.shape[0]) | |
| if ratio > 1 and custom_hover_data.shape[0] * ratio == trace_len: | |
| custom_hover_data = repeat(custom_hover_data, ratio, axis=0) | |
| self.fig.update_traces(hovertemplate=tooltips, customdata=custom_hover_data) | |
| else: | |
| # If there is a single trace but the trace x-array was expanded | |
| # (for example spectrum plotting repeats each mz value several times | |
| # to draw stems), expand the custom_hover_data to match the trace length. | |
| trace = self.fig.data[0] | |
| trace_len = len(trace.x) if hasattr(trace, "x") else None | |
| if ( | |
| trace_len is not None | |
| and custom_hover_data is not None | |
| and custom_hover_data.shape[0] > 0 | |
| and custom_hover_data.shape[0] != trace_len | |
| ): | |
| # If trace length is an integer multiple of custom data rows, | |
| # repeat each row accordingly. | |
| ratio = int(trace_len / custom_hover_data.shape[0]) | |
| if ratio > 1 and custom_hover_data.shape[0] * ratio == trace_len: | |
| custom_hover_data = repeat(custom_hover_data, ratio, axis=0) | |
| self.fig.update_traces(hovertemplate=tooltips, customdata=custom_hover_data) |
🤖 Prompt for AI Agents
In `@pyopenms_viz/_plotly/core.py` around lines 163 - 180, The code can raise
ZeroDivisionError when custom_hover_data has zero rows; in the block that
computes ratio = int(trace_len / custom_hover_data.shape[0]) (used to possibly
repeat custom_hover_data via repeat) add a defensive guard to ensure
custom_hover_data.shape[0] > 0 before performing the division and only
compute/repeat when that condition holds (otherwise skip expansion or leave
custom_hover_data as-is), then call self.fig.update_traces(...) with the
unchanged custom_hover_data.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyopenms_viz/testing/MatplotlibSnapshotExtension.py (1)
65-85: Fix the return type annotation: method returnsbytes, notstr.The
serializemethod returnsbuf.getvalue()which isbytes, but the signature declares-> str. This inconsistency will cause type checker errors and is inconsistent with the other snapshot extensions in this PR (Plotly and Bokeh both correctly declare-> bytes).🐛 Proposed fix for type annotation and docstring
- def serialize(self, data: SerializableData, **kwargs: Any) -> str: + def serialize(self, data: SerializableData, **kwargs: Any) -> bytes: """ Serialize the matplotlib Axis or Figure object to a png Args: data (SerializableData): Matplotlib data to serialize, should be an axis object Returns: - str: Image object + bytes: PNG image bytes """
♻️ Duplicate comments (1)
pyopenms_viz/_plotly/core.py (1)
169-177: Guard against zero-row hoverdata before dividing.
custom_hover_data.shape[0]can be 0, which raisesZeroDivisionErrorat Line 176. Add a >0 check before computingratioor repeating.🔧 Proposed fix
if ( trace_len is not None and custom_hover_data is not None + and custom_hover_data.shape[0] > 0 and custom_hover_data.shape[0] != trace_len ): # If trace length is an integer multiple of custom data rows, # repeat each row accordingly. ratio = trace_len // custom_hover_data.shape[0]
🧹 Nitpick comments (1)
pyopenms_viz/testing/MatplotlibSnapshotExtension.py (1)
19-38: Consider catching more specific exceptions for type detection.The broad
except Exceptionblocks work for fallback logic, but catching more specific exceptions (e.g.,TypeError,AttributeError, or PIL-specific exceptions) would be safer and avoid masking unexpected errors. However, given this is test infrastructure and the intent is clear, this is acceptable as-is.♻️ Optional: More specific exception handling
try: serialized_img = Image.open(BytesIO(serialized_data)) - except Exception: + except (TypeError, AttributeError, IOError): # If already an Image object, use it directly serialized_img = serialized_data try: snapshot_img = Image.open(BytesIO(snapshot_data)) - except Exception: + except (TypeError, AttributeError, IOError): snapshot_img = snapshot_data
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pyopenms_viz/_plotly/core.pypyopenms_viz/testing/BokehSnapshotExtension.pypyopenms_viz/testing/MatplotlibSnapshotExtension.pypyopenms_viz/testing/PlotlySnapshotExtension.py
🧰 Additional context used
🧬 Code graph analysis (1)
pyopenms_viz/testing/PlotlySnapshotExtension.py (4)
pyopenms_viz/testing/BokehSnapshotExtension.py (1)
serialize(168-178)pyopenms_viz/testing/MatplotlibSnapshotExtension.py (1)
serialize(65-85)pyopenms_viz/testing/PandasSnapshotExtension.py (1)
serialize(54-55)pyopenms_viz/testing/NumpySnapshotExtension.py (1)
serialize(59-60)
🪛 Ruff (0.14.11)
pyopenms_viz/testing/PlotlySnapshotExtension.py
98-98: Unused method argument: kwargs
(ARG002)
pyopenms_viz/testing/BokehSnapshotExtension.py
168-168: Unused method argument: kwargs
(ARG002)
pyopenms_viz/testing/MatplotlibSnapshotExtension.py
23-23: Do not catch blind exception: Exception
(BLE001)
29-29: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (3)
- GitHub Check: build
- GitHub Check: test (windows-latest, 3.12)
- GitHub Check: test (ubuntu-latest, 3.12)
🔇 Additional comments (6)
pyopenms_viz/testing/PlotlySnapshotExtension.py (3)
17-30: LGTM! Robust bytes/string handling for JSON comparison.The defensive decoding of both
serialized_dataandsnapshot_dataensures compatibility whether the data arrives as bytes or strings. This aligns well with the binary I/O changes throughout the file.
72-80: LGTM! Consistent binary I/O semantics.Reading in binary mode and writing bytes directly is consistent with the
serialize()method now returning bytes. The defensivestr → bytesconversion inwrite_snapshot_collectionhandles edge cases gracefully.Also applies to: 92-96
98-108: LGTM! Serialization correctly returns UTF-8 encoded bytes.The implementation properly encodes the JSON output to bytes, matching the updated return type annotation. The
kwargsparameter is inherited from theSingleFileSnapshotExtensioninterface and is acceptable to leave unused for interface compliance.pyopenms_viz/testing/BokehSnapshotExtension.py (3)
78-83: LGTM! Defensive bytes decoding for HTML parsing.Properly handles both bytes and string inputs, ensuring compatibility with the binary read operations.
142-150: LGTM! Consistent binary I/O operations.Reading and writing in binary mode aligns with the
serialize()method returning bytes. The defensivestr → bytesencoding inwrite_snapshot_collectionhandles any edge cases.Also applies to: 162-166
168-178: LGTM! Clean bytes serialization.The implementation correctly encodes HTML to UTF-8 bytes with proper type annotation and docstring. The
kwargsparameter is part of the inherited interface.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 63 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@test/__snapshots__/test_spectrum/test_mirror_spectrum`[ms_plotly].raw:
- Around line 8-15: pyproject.toml currently lists "plotly" without a version
while requirements.txt pins plotly==5.24.1; update the pyproject.toml dependency
entry for plotly to match the pinned version (5.24.1) — e.g., set the "plotly"
dependency in the pyproject.toml dependencies section to "5.24.1" (or the same
specifier used in requirements.txt) so both manifests consistently pin plotly
and ensure stable snapshots.
In
`@test/__snapshots__/test_spectrum/test_spectrum_binning`[ms_bokeh-kwargs0].raw:
- Around line 1-61: convert_for_line_plots() is currently stripping all columns
except x, y and group before building the Bokeh ColumnDataSource, which removes
metadata like native_id referenced by the HoverTool tooltips; update
convert_for_line_plots() (or the helper that builds the ColumnDataSource) to
preserve any non-index metadata columns (e.g., native_id) when present so the
ColumnDataSource contains x, y, group plus metadata fields, or alternatively
remove native_id from the HoverTool tooltips construction (where HoverTool is
created) so tooltips only reference fields guaranteed to exist.
In
`@test/__snapshots__/test_spectrum/test_spectrum_binning`[ms_plotly-kwargs0].raw:
- Around line 8-15: The snapshot contains misaligned binary arrays: the "x"
field (dtype f8) decodes to 28 values while "y" (dtype i1) decodes to 27, and
neither matches the 9 entries in "customdata" in test_spectrum_binning; fix by
ensuring the generator producing these arrays creates matching lengths—make "x"
and "y" the same length and align them with the number of "customdata" entries
(e.g., adjust the data-producing code in the test or fixture so "x", "y" and
"customdata" all have the same count), then re-encode the bdata for "x" and "y"
to reflect the corrected arrays and update the snapshot for
test_spectrum_binning.
In
`@test/__snapshots__/test_spectrum/test_spectrum_plot_with_peak_color`[ms_bokeh-kwargs1].raw:
- Line 15: The stored snapshot references Bokeh 3.4.1 in the script tag and must
be updated to match the current test environment (Bokeh 3.6.x); regenerate the
snapshot for test_spectrum_plot_with_peak_color[ms_bokeh-kwargs1] so the script
src reflects the resolved Bokeh version (e.g., update the <script ...
bokeh-3.4.1.min.js> entry to bokeh-3.6.x.min.js) by re-running the snapshot test
or snapshot update command to produce the new expected output.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (58)
test/__snapshots__/test_spectrum/test_mirror_spectrum[ms_bokeh].htmltest/__snapshots__/test_spectrum/test_mirror_spectrum[ms_bokeh].rawtest/__snapshots__/test_spectrum/test_mirror_spectrum[ms_matplotlib].rawtest/__snapshots__/test_spectrum/test_mirror_spectrum[ms_plotly].rawtest/__snapshots__/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs0].htmltest/__snapshots__/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs0].rawtest/__snapshots__/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs1].htmltest/__snapshots__/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs1].rawtest/__snapshots__/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs2].htmltest/__snapshots__/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs2].rawtest/__snapshots__/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs3].htmltest/__snapshots__/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs3].rawtest/__snapshots__/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs4].htmltest/__snapshots__/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs4].rawtest/__snapshots__/test_spectrum/test_spectrum_binning[ms_matplotlib-kwargs0].rawtest/__snapshots__/test_spectrum/test_spectrum_binning[ms_matplotlib-kwargs1].rawtest/__snapshots__/test_spectrum/test_spectrum_binning[ms_matplotlib-kwargs2].rawtest/__snapshots__/test_spectrum/test_spectrum_binning[ms_matplotlib-kwargs3].rawtest/__snapshots__/test_spectrum/test_spectrum_binning[ms_matplotlib-kwargs4].rawtest/__snapshots__/test_spectrum/test_spectrum_binning[ms_plotly-kwargs0].rawtest/__snapshots__/test_spectrum/test_spectrum_binning[ms_plotly-kwargs1].rawtest/__snapshots__/test_spectrum/test_spectrum_binning[ms_plotly-kwargs2].rawtest/__snapshots__/test_spectrum/test_spectrum_binning[ms_plotly-kwargs3].rawtest/__snapshots__/test_spectrum/test_spectrum_binning[ms_plotly-kwargs4].rawtest/__snapshots__/test_spectrum/test_spectrum_plot[ms_bokeh-kwargs0].htmltest/__snapshots__/test_spectrum/test_spectrum_plot[ms_bokeh-kwargs0].rawtest/__snapshots__/test_spectrum/test_spectrum_plot[ms_bokeh-kwargs1].htmltest/__snapshots__/test_spectrum/test_spectrum_plot[ms_bokeh-kwargs1].rawtest/__snapshots__/test_spectrum/test_spectrum_plot[ms_bokeh-kwargs2].htmltest/__snapshots__/test_spectrum/test_spectrum_plot[ms_bokeh-kwargs2].rawtest/__snapshots__/test_spectrum/test_spectrum_plot[ms_bokeh-kwargs3].htmltest/__snapshots__/test_spectrum/test_spectrum_plot[ms_bokeh-kwargs3].rawtest/__snapshots__/test_spectrum/test_spectrum_plot[ms_matplotlib-kwargs0].rawtest/__snapshots__/test_spectrum/test_spectrum_plot[ms_matplotlib-kwargs1].rawtest/__snapshots__/test_spectrum/test_spectrum_plot[ms_matplotlib-kwargs2].rawtest/__snapshots__/test_spectrum/test_spectrum_plot[ms_matplotlib-kwargs3].rawtest/__snapshots__/test_spectrum/test_spectrum_plot[ms_plotly-kwargs0].rawtest/__snapshots__/test_spectrum/test_spectrum_plot[ms_plotly-kwargs1].rawtest/__snapshots__/test_spectrum/test_spectrum_plot[ms_plotly-kwargs2].rawtest/__snapshots__/test_spectrum/test_spectrum_plot[ms_plotly-kwargs3].rawtest/__snapshots__/test_spectrum/test_spectrum_plot_with_peak_color[ms_bokeh-kwargs0].rawtest/__snapshots__/test_spectrum/test_spectrum_plot_with_peak_color[ms_bokeh-kwargs1].rawtest/__snapshots__/test_spectrum/test_spectrum_plot_with_peak_color[ms_matplotlib-kwargs0].rawtest/__snapshots__/test_spectrum/test_spectrum_plot_with_peak_color[ms_matplotlib-kwargs1].rawtest/__snapshots__/test_spectrum/test_spectrum_plot_with_peak_color[ms_plotly-kwargs0].rawtest/__snapshots__/test_spectrum/test_spectrum_plot_with_peak_color[ms_plotly-kwargs1].rawtest/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_bokeh-kwargs0].htmltest/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_bokeh-kwargs0].rawtest/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_bokeh-kwargs1].htmltest/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_bokeh-kwargs1].rawtest/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_bokeh-kwargs2].htmltest/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_bokeh-kwargs2].rawtest/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_matplotlib-kwargs0].rawtest/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_matplotlib-kwargs1].rawtest/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_matplotlib-kwargs2].rawtest/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_plotly-kwargs0].rawtest/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_plotly-kwargs1].rawtest/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_plotly-kwargs2].raw
💤 Files with no reviewable changes (13)
- test/snapshots/test_spectrum/test_spectrum_with_annotations[ms_bokeh-kwargs2].html
- test/snapshots/test_spectrum/test_spectrum_plot[ms_bokeh-kwargs3].html
- test/snapshots/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs0].html
- test/snapshots/test_spectrum/test_spectrum_plot[ms_bokeh-kwargs2].html
- test/snapshots/test_spectrum/test_mirror_spectrum[ms_bokeh].html
- test/snapshots/test_spectrum/test_spectrum_with_annotations[ms_bokeh-kwargs1].html
- test/snapshots/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs4].html
- test/snapshots/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs1].html
- test/snapshots/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs2].html
- test/snapshots/test_spectrum/test_spectrum_plot[ms_bokeh-kwargs1].html
- test/snapshots/test_spectrum/test_spectrum_plot[ms_bokeh-kwargs0].html
- test/snapshots/test_spectrum/test_spectrum_with_annotations[ms_bokeh-kwargs0].html
- test/snapshots/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs3].html
✅ Files skipped from review due to trivial changes (1)
- test/snapshots/test_spectrum/test_spectrum_with_annotations[ms_bokeh-kwargs2].raw
⏰ 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). (1)
- GitHub Check: CodeQL analysis (python)
🔇 Additional comments (36)
test/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_plotly-kwargs1].raw (2)
8-15: Snapshot update looks consistent with binary serialization.
No issues flagged here.
461-472: Template update for scattermap looks fine.
No concerns from the snapshot perspective.test/__snapshots__/test_spectrum/test_mirror_spectrum[ms_bokeh].raw (1)
1-61: Snapshot update looks consistent.
No issues spotted in the serialized Bokeh HTML; the snapshot appears deterministic and aligned with expected embedding behavior.test/__snapshots__/test_spectrum/test_spectrum_plot[ms_plotly-kwargs3].raw (2)
8-15: Binary-encoded x/y arrays look consistent with the new snapshot format.No issues spotted in the serialized structure for
xandy.
461-472: scattermap template block addition looks fine.This aligns with the updated Plotly template expectations reflected in the snapshot.
test/__snapshots__/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs4].raw (1)
1-61: Snapshot update looks consistent with raw HTML serialization.No issues spotted in this updated snapshot artifact.
test/__snapshots__/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs2].raw (1)
1-61: Auto-generated test snapshot file looks valid.This is a Bokeh HTML snapshot file for the spectrum binning test. The structure follows standard Bokeh embedding patterns with CDN script loading, embedded JSON document data, and the polling-based embedding script. Since this is auto-generated test output used for snapshot comparison, no manual changes are expected.
test/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_bokeh-kwargs1].raw (1)
1-61: LGTM - Well-structured Bokeh test snapshot.This is an auto-generated Bokeh HTML snapshot for testing spectrum annotations. The file correctly includes:
- Self-contained BokehJS embedding with proper fallback polling for library availability
- Spectrum data with annotation labels (e.g.,
"100.5332\ncustom3") in the JSON document- Hover tool configuration for m/z and intensity tooltips
This aligns with the PR's annotation fix and the broader snapshot handling changes mentioned in the summary.
test/__snapshots__/test_spectrum/test_spectrum_plot[ms_bokeh-kwargs3].raw (1)
1-61: Test snapshot file appears valid.This is an auto-generated Bokeh HTML snapshot capturing the expected visualization output. The structure follows standard Bokeh embedding patterns with proper CDN loading, JSON document definition, and fallback embedding logic.
The snapshot includes the expected spectrum annotations (Label objects for m/z values like "100.5332", "74.1324", etc.), which aligns with the PR's goal of verifying annotation rendering.
Note: The snapshot contains dynamically generated UUIDs (e.g., line 21, 23, 32-33). If your snapshot comparison is exact-match based, ensure these UUIDs remain stable across test runs or that your comparison logic accounts for them.
test/__snapshots__/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs1].raw (1)
1-61: Snapshot update looks consistent.No issues to flag in this snapshot content.
test/__snapshots__/test_spectrum/test_spectrum_plot[ms_bokeh-kwargs1].raw (1)
1-61: LGTM - Valid Bokeh test snapshot for multi-trace spectrum.This auto-generated snapshot correctly captures a 3-trace Mass Spectrum visualization with annotation labels, validating the fix for spectrum annotations.
test/__snapshots__/test_spectrum/test_spectrum_plot[ms_bokeh-kwargs2].raw (1)
1-61: LGTM - Valid Bokeh test snapshot for color-annotated spectrum.This snapshot correctly captures a 4-trace visualization with explicit color annotations, using float-typed intensity values. The annotation labels are properly embedded.
test/__snapshots__/test_spectrum/test_spectrum_plot[ms_bokeh-kwargs0].raw (1)
1-61: LGTM - Critical test snapshot for single-trace annotation fix.This snapshot is central to validating the PR's fix for issue
#88. It correctly captures a single-trace spectrum with all 5 annotation labels properly embedded (Labels p1047-p1051), confirming that annotations now render correctly for single-trace spectra.test/__snapshots__/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs3].raw (1)
1-61: Snapshot file verified.This is an auto-generated Bokeh HTML snapshot used for regression testing with the correct structure.
test/__snapshots__/test_spectrum/test_spectrum_plot_with_peak_color[ms_bokeh-kwargs1].raw (2)
21-24: Snapshot regeneration changes look consistent.The updated UUIDs and document IDs are expected when regenerating Bokeh snapshots. Internal references are consistent:
- Root id
p1384in the div'sdata-root-idcorrectly maps to the root inrender_items- Document JSON structure and visualization data remain intact
32-33: Embed references are internally consistent.The JavaScript embed code correctly references the updated IDs:
getElementByIdtargets the correct JSON script elementrender_itemsdocid matches the JSON document key- Root mapping (
p1384→da160197-...) aligns with the div elementtest/__snapshots__/test_spectrum/test_spectrum_plot[ms_plotly-kwargs2].raw (2)
9-16: Snapshot binary encoding update looks consistent.These blocks align with the new bytes-based Plotly serialization and appear coherent for the scatter traces.
Also applies to: 88-95, 122-129, 171-178
609-619: No action required. The repository's Plotly version (5.24.1) fully supportsscattermap, which was introduced in Plotly.py v5.24.0. The snapshot is compatible with the pinned dependency and does not present version-dependent failure risks.Likely an incorrect or invalid review comment.
test/__snapshots__/test_spectrum/test_mirror_spectrum[ms_plotly].raw (2)
66-80: Second trace correctly omits customdata for mirror visualization.The mirrored trace uses the same binary encoding and appropriately lacks
customdata/hovertemplatesince hover info on the primary trace is sufficient for mirror spectrum plots.
476-487: Template now includes bothscattermapandscattermapboxtrace types.This addition reflects Plotly's transition to MapLibre-powered map traces. The
scattermaptype is a new MapLibre-based alternative toscattermapboxfor map visualizations. Note thatheatmapgl(used in other snapshots) serves a different purpose and remains unchanged. The project should ensure the unconstrainedplotlydependency inpyproject.tomlis compatible with the snapshot expectations, as Plotly 5.24.0+ is required forscattermapsupport.Likely an incorrect or invalid review comment.
test/__snapshots__/test_spectrum/test_spectrum_plot[ms_plotly-kwargs1].raw (2)
9-193: Snapshot encoding update looks consistent.
The binarydtype/bdatax/y encoding while preservingcustomdataand hover templates matches Plotly’s array serialization style.
590-600: Scattermap template addition looks good.
Including thescattermapdefaults in the template keeps trace rendering consistent with the updated Plotly schema.test/__snapshots__/test_spectrum/test_spectrum_binning[ms_plotly-kwargs3].raw (2)
8-15: Binary x/y encoding aligns with new serialization.
The switch todtype/bdatafor coordinates is consistent with Plotly’s binary array format.
461-472: Scattermap template defaults are fine.
This template addition is consistent with the other Plotly snapshot updates.test/__snapshots__/test_spectrum/test_spectrum_binning[ms_plotly-kwargs2].raw (2)
8-15: Binary coordinate encoding looks correct.
Thedtype/bdatashift for x/y matches the updated Plotly snapshot format.
461-472: Scattermap template block is consistent with the new format.
No concerns with this addition.test/__snapshots__/test_spectrum/test_spectrum_plot_with_peak_color[ms_plotly-kwargs1].raw (2)
9-203: Binary x/y serialization looks consistent across traces.
Thedtype/bdatablocks for both traces align with the updated Plotly snapshot format.
616-627: Scattermap template defaults look fine.
This addition matches the other Plotly snapshot updates.test/__snapshots__/test_spectrum/test_spectrum_binning[ms_plotly-kwargs4].raw (2)
8-15: Binary coordinate encoding is consistent.
The snapshot reflects the expected Plotlydtype/bdataserialization.
461-472: Scattermap template addition looks good.
No issues noted with the template expansion.test/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_plotly-kwargs2].raw (1)
9-277: Snapshot update aligns with the new Plotly serialization.Binary-encoded coordinate blocks and the added scattermap template look consistent with the updated snapshot format.
Also applies to: 674-685
test/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_plotly-kwargs0].raw (1)
9-278: Snapshot update aligns with the new Plotly serialization.Binary-encoded coordinate blocks and the scattermap template addition are consistent with the new snapshot format.
Also applies to: 674-685
test/__snapshots__/test_spectrum/test_spectrum_binning[ms_plotly-kwargs1].raw (1)
8-65: Snapshot update aligns with the new Plotly serialization.The binary-encoded coordinate data and scattermap template entry look consistent with the updated snapshot format.
Also applies to: 461-472
test/__snapshots__/test_spectrum/test_spectrum_plot_with_peak_color[ms_plotly-kwargs0].raw (1)
9-219: Snapshot update aligns with the new Plotly serialization.Binary-encoded coordinates and the scattermap template addition look consistent with the serializer changes.
Also applies to: 616-627
test/__snapshots__/test_spectrum/test_spectrum_plot[ms_plotly-kwargs0].raw (1)
8-65: Snapshot update aligns with the new Plotly serialization.Binary-encoded coordinate blocks and the scattermap template entry look consistent with the updated snapshot format.
Also applies to: 461-472
test/__snapshots__/test_spectrum/test_spectrum_binning[ms_plotly-kwargs0].raw (1)
461-472: Template addition aligns with Plotly conventions.The new
scattermaptrace type follows the same structure as existing trace definitions (e.g.,scattermapbox,scattergeo). This is likely introduced by a Plotly version update adding native support for this trace type.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| "x": { | ||
| "dtype": "f8", | ||
| "bdata": "O99PjZd+SUA730+Nl35JQDvfT42XfklAY3\u002fZPXmIUkBjf9k9eYhSQGN\u002f2T15iFJAio7k8h8iWUCKjuTyHyJZQIqO5PIfIllAexSuR+FiWUB7FK5H4WJZQHsUrkfhYllALpCg+DGiWUAukKD4MaJZQC6QoPgxollAZmZmZmYGZEBmZmZmZgZkQGZmZmZmBmRAsp3vp8b\u002fZUCyne+nxv9lQLKd76fG\u002f2VAWvW52ooNaUBa9bnaig1pQFr1udqKDWlAWvW52oodaUBa9bnaih1pQFr1udqKHWlA" | ||
| }, | ||
| "y": { | ||
| "dtype": "f8", | ||
| "bdata": "AAAAAAAAAAAAAAAAAABEQAAAAAAAAAAAAAAAAAAAAAAAAAAAAABUQAAAAAAAAAAAAAAAAAAAAAAAAAAAAABZQAAAAAAAAAAAAAAAAAAAAAAAAAAAAABIQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA4QAAAAAAAAAAAAAAAAAAAAAAAAAAAAABKQAAAAAAAAAAAAAAAAAAAAAAAAAAAAABAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAABRQAAAAAAAAAAAAAAAAAAAAAABAAAAAAA8QAAAAAAAAAAA" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find package files and Plotly version
find . -type f \( -name "package.json" -o -name "requirements.txt" -o -name "pyproject.toml" \) | head -20Repository: OpenMS/pyopenms_viz
Length of output: 149
🏁 Script executed:
# Check if the snapshot file exists and examine its structure
ls -la test/__snapshots__/test_spectrum/test_mirror_spectrum* 2>/dev/null || echo "Snapshot file path needs verification"Repository: OpenMS/pyopenms_viz
Length of output: 422
🏁 Script executed:
# Find any Plotly-related dependencies
rg "plotly" --type json --type toml --type txt -i | head -20Repository: OpenMS/pyopenms_viz
Length of output: 418
🏁 Script executed:
# Examine the snapshot file structure and content
head -100 test/__snapshots__/test_spectrum/test_mirror_spectrum[ms_plotly].rawRepository: OpenMS/pyopenms_viz
Length of output: 3033
🏁 Script executed:
# Check line count to understand file size
wc -l test/__snapshots__/test_spectrum/test_mirror_spectrum[ms_plotly].rawRepository: OpenMS/pyopenms_viz
Length of output: 136
🏁 Script executed:
# Search for the specific lines mentioned in the review (lines 8-15)
sed -n '1,20p' test/__snapshots__/test_spectrum/test_mirror_spectrum[ms_plotly].rawRepository: OpenMS/pyopenms_viz
Length of output: 974
🌐 Web query:
Plotly.js 5.24.1 binary data serialization dtype bdata format
💡 Result:
Short answer
- "bdata" is Plotly.js's typed-array serialization: an object with keys dtype (e.g. "f8","f4","i4","u4","i2","u2","i1","u1","u1c"), bdata (a base64-encoded ArrayBuffer or raw ArrayBuffer), and optional shape for multi‑dim arrays. It is used so Plotly can accept binary typed arrays (faster/more compact) instead of JSON arrays. [1][2]
What each token means
- dtype → numeric type mapping:
- f8 → Float64Array
- f4 → Float32Array
- i4 → Int32Array
- u4 → Uint32Array
- i2 → Int16Array
- u2 → Uint16Array
- i1 → Int8Array
- u1 → Uint8Array
- u1c → Uint8ClampedArray
- bdata → base64 string representing the ArrayBuffer bytes (or an actual ArrayBuffer when sent in memory).
- shape → comma-separated dimensions for multidimensional arrays (optional for 1‑D). [1]
Notes and gotchas
- Plotly.py (and Dash) will base64-encode NumPy arrays when passing them to Plotly.js; the Python side may add an internal _inputArray wrapper when serializing. [2]
- Endianness can affect interoperability (big-endian NumPy dtypes have caused issues in the past). If you see odd values, check byte-order/ dtype conversion before encoding. [3]
Quick JS decode example (concept)
- Given an object obj = {dtype: 'f8', bdata: '...base64...'}:
- bytes = Uint8Array.from(atob(obj.bdata), c => c.charCodeAt(0));
- buffer = bytes.buffer;
- arr = new Float64Array(buffer); // use type matching obj.dtype
- if obj.shape present, reshape accordingly.
Sources
[1] Plotly.js typed-array/typed data spec in source (coerce/typed-array handling).
[2] Plotly.py performance guide describing base64-encoded NumPy arrays → Plotly.js typed arrays.
[3] Plotly community reports about dtype/binary encoding and endianness issues.
🌐 Web query:
Plotly typed array bdata f8 float64 serialization
💡 Result:
Short answer
- In Plotly's typed-array spec, dtype "f8" = float64 (JS Float64Array). bdata is the array buffer either as a base64 string or as an ArrayBuffer; shape is optional for 1‑D or required for multi‑D. Plotly.py encodes NumPy arrays into this spec (dtype short names like "f8") and Plotly.js decodes them back into typed arrays for rendering. [1][2][3]
Minimal JSON example
- 1-D float64:
{"dtype":"f8","bdata":""} - 2-D (shape as rows,cols):
{"dtype":"f8","bdata":"","shape":"5,100"}
Notes
- bdata may be an ArrayBuffer (binary) or a base64 string; Plotly.js will b64-decode if necessary and construct a Float64Array using the BYTES_PER_ELEMENT of the f8 type. [2][3]
- Plotly.py maps NumPy dtypes to short dtype names ("float64" -> "f8") when producing the typed-array spec. [1]
Sources
- Plotly.py dtype mapping / to_typed_array_spec implementation. [1]
- Plotly.js typed-array spec description (coerce/decodeTypedArraySpec). [2]
- Plotly.js runtime decode implementation (creates typed arrays from bdata). [3]
Ensure Plotly version constraint is consistent across all configuration files.
The snapshot correctly shows Plotly's typed-array serialization format (dtype: "f8" for 64-bit floats, bdata for base64-encoded binary data). This is standard behavior when Plotly.py encodes NumPy arrays for rendering.
However, Plotly's version is pinned to 5.24.1 in requirements.txt but lacks a version constraint in pyproject.toml (specifies "plotly" without version). Update pyproject.toml to match the version pin in requirements.txt to ensure snapshot stability across environments.
🤖 Prompt for AI Agents
In `@test/__snapshots__/test_spectrum/test_mirror_spectrum`[ms_plotly].raw around
lines 8 - 15, pyproject.toml currently lists "plotly" without a version while
requirements.txt pins plotly==5.24.1; update the pyproject.toml dependency entry
for plotly to match the pinned version (5.24.1) — e.g., set the "plotly"
dependency in the pyproject.toml dependencies section to "5.24.1" (or the same
specifier used in requirements.txt) so both manifests consistently pin plotly
and ensure stable snapshots.
| <!DOCTYPE html> | ||
| <html lang="en"> | ||
| <head> | ||
| <meta charset="utf-8"> | ||
| <title>Bokeh Application</title> | ||
| <style> | ||
| html, body { | ||
| box-sizing: border-box; | ||
| display: flow-root; | ||
| height: 100%; | ||
| margin: 0; | ||
| padding: 0; | ||
| } | ||
| </style> | ||
| <script type="text/javascript" src="https://cdn.bokeh.org/bokeh/release/bokeh-3.4.1.min.js"></script> | ||
| <script type="text/javascript"> | ||
| Bokeh.set_log_level("info"); | ||
| </script> | ||
| </head> | ||
| <body> | ||
| <div id="d5627e74-de6b-4c45-9b4e-869ae55955b5" data-root-id="p1454" style="display: contents;"></div> | ||
|
|
||
| <script type="application/json" id="cbb7b83d-4a93-4e1d-b75f-64f7c08ed51d"> | ||
| {"4678ae8c-f3e7-4997-a76d-6ff7e04d46ee":{"version":"3.4.1","title":"Bokeh Application","roots":[{"type":"object","name":"Figure","id":"p1454","attributes":{"width":500,"height":500,"x_range":{"type":"object","name":"Range1d","id":"p1506","attributes":{"start":40.791199999999996,"end":241.10784}},"y_range":{"type":"object","name":"Range1d","id":"p1507","attributes":{"end":28.75}},"x_scale":{"type":"object","name":"LinearScale","id":"p1464"},"y_scale":{"type":"object","name":"LinearScale","id":"p1465"},"title":{"type":"object","name":"Title","id":"p1457","attributes":{"text":"Mass Spectrum","text_font_size":"18pt"}},"renderers":[{"type":"object","name":"GlyphRenderer","id":"p1496","attributes":{"data_source":{"type":"object","name":"ColumnDataSource","id":"p1487","attributes":{"selected":{"type":"object","name":"Selection","id":"p1488","attributes":{"indices":[],"line_indices":[]}},"selection_policy":{"type":"object","name":"UnionRenderers","id":"p1489"},"data":{"type":"map","entries":[["index",{"type":"ndarray","array":{"type":"bytes","data":"AAAAAAAAAAAAAAAAAQAAAAEAAAABAAAAAgAAAAIAAAACAAAAAwAAAAMAAAADAAAABAAAAAQAAAAEAAAABQAAAAUAAAAFAAAABgAAAAYAAAAGAAAABwAAAAcAAAAHAAAACAAAAAgAAAAIAAAA"},"shape":[27],"dtype":"int32","order":"little"}],["mz",{"type":"ndarray","array":{"type":"bytes","data":"O99PjZd+SUA730+Nl35JQDvfT42XfklAY3/ZPXmIUkBjf9k9eYhSQGN/2T15iFJAio7k8h8iWUCKjuTyHyJZQIqO5PIfIllAexSuR+FiWUB7FK5H4WJZQHsUrkfhYllALpCg+DGiWUAukKD4MaJZQC6QoPgxollAZmZmZmYGZEBmZmZmZgZkQGZmZmZmBmRAsp3vp8b/ZUCyne+nxv9lQLKd76fG/2VAWvW52ooNaUBa9bnaig1pQFr1udqKDWlAWvW52oodaUBa9bnaih1pQFr1udqKHWlA"},"shape":[27],"dtype":"float64","order":"little"}],["intensity",{"type":"ndarray","array":{"type":"bytes","data":"AAAAAAoAAAAAAAAAAAAAABQAAAAAAAAAAAAAABkAAAAAAAAAAAAAAAwAAAAAAAAAAAAAAAYAAAAAAAAAAAAAAA0AAAAAAAAAAAAAAAgAAAAAAAAAAAAAABEAAAAAAAAAAAAAAAcAAAAAAAAA"},"shape":[27],"dtype":"int32","order":"little"}]]}}},"view":{"type":"object","name":"CDSView","id":"p1497","attributes":{"filter":{"type":"object","name":"AllIndices","id":"p1498"}}},"glyph":{"type":"object","name":"Line","id":"p1493","attributes":{"x":{"type":"field","field":"mz"},"y":{"type":"field","field":"intensity"},"line_color":"#4575B4"}},"nonselection_glyph":{"type":"object","name":"Line","id":"p1494","attributes":{"x":{"type":"field","field":"mz"},"y":{"type":"field","field":"intensity"},"line_color":"#4575B4","line_alpha":0.1}},"muted_glyph":{"type":"object","name":"Line","id":"p1495","attributes":{"x":{"type":"field","field":"mz"},"y":{"type":"field","field":"intensity"},"line_color":"#4575B4","line_alpha":0.2}}}}],"toolbar":{"type":"object","name":"Toolbar","id":"p1463","attributes":{"tools":[{"type":"object","name":"PanTool","id":"p1476"},{"type":"object","name":"WheelZoomTool","id":"p1477","attributes":{"renderers":"auto"}},{"type":"object","name":"BoxZoomTool","id":"p1478","attributes":{"overlay":{"type":"object","name":"BoxAnnotation","id":"p1479","attributes":{"syncable":false,"level":"overlay","visible":false,"left":{"type":"number","value":"nan"},"right":{"type":"number","value":"nan"},"top":{"type":"number","value":"nan"},"bottom":{"type":"number","value":"nan"},"left_units":"canvas","right_units":"canvas","top_units":"canvas","bottom_units":"canvas","line_color":"black","line_alpha":1.0,"line_width":2,"line_dash":[4,4],"fill_color":"lightgrey","fill_alpha":0.5}}}},{"type":"object","name":"SaveTool","id":"p1484"},{"type":"object","name":"ResetTool","id":"p1485"},{"type":"object","name":"HelpTool","id":"p1486"},{"type":"object","name":"HoverTool","id":"p1499","attributes":{"renderers":"auto","tooltips":[["m/z","@mz"],["intensity","@intensity"],["native id","@native_id"]]}}]}},"toolbar_location":"above","left":[{"type":"object","name":"LinearAxis","id":"p1471","attributes":{"ticker":{"type":"object","name":"BasicTicker","id":"p1472","attributes":{"mantissas":[1,2,5]}},"formatter":{"type":"object","name":"BasicTickFormatter","id":"p1473"},"axis_label":"Intensity","axis_label_text_font_size":"16pt","major_label_policy":{"type":"object","name":"AllLabels","id":"p1474"},"major_label_text_font_size":"14pt"}}],"below":[{"type":"object","name":"LinearAxis","id":"p1466","attributes":{"ticker":{"type":"object","name":"BasicTicker","id":"p1467","attributes":{"mantissas":[1,2,5]}},"formatter":{"type":"object","name":"BasicTickFormatter","id":"p1468"},"axis_label":"mass-to-charge","axis_label_text_font_size":"16pt","major_label_policy":{"type":"object","name":"AllLabels","id":"p1469"},"major_label_text_font_size":"14pt"}}],"center":[{"type":"object","name":"Grid","id":"p1470","attributes":{"axis":{"id":"p1466"}}},{"type":"object","name":"Grid","id":"p1475","attributes":{"dimension":1,"axis":{"id":"p1471"}}},{"type":"object","name":"Label","id":"p1500","attributes":{"text":"100.5332","text_color":"black","text_font_size":"13pt","x":100.5332,"y":25,"x_offset":1}},{"type":"object","name":"Label","id":"p1501","attributes":{"text":"74.1324","text_color":"black","text_font_size":"13pt","x":74.1324,"y":20,"x_offset":1}},{"type":"object","name":"Label","id":"p1502","attributes":{"text":"200.4232","text_color":"black","text_font_size":"13pt","x":200.4232,"y":17,"x_offset":1}},{"type":"object","name":"Label","id":"p1503","attributes":{"text":"160.2","text_color":"black","text_font_size":"13pt","x":160.2,"y":13,"x_offset":1}},{"type":"object","name":"Label","id":"p1504","attributes":{"text":"101.545","text_color":"black","text_font_size":"13pt","x":101.545,"y":12,"x_offset":1}},{"type":"object","name":"Span","id":"p1505","attributes":{"location":0,"line_color":"#EEEEEE","line_width":2}}],"min_border":0}}]}} | ||
| </script> | ||
| <script type="text/javascript"> | ||
| (function() { | ||
| const fn = function() { | ||
| Bokeh.safely(function() { | ||
| (function(root) { | ||
| function embed_document(root) { | ||
| const docs_json = document.getElementById('cbb7b83d-4a93-4e1d-b75f-64f7c08ed51d').textContent; | ||
| const render_items = [{"docid":"4678ae8c-f3e7-4997-a76d-6ff7e04d46ee","roots":{"p1454":"d5627e74-de6b-4c45-9b4e-869ae55955b5"},"root_ids":["p1454"]}]; | ||
| root.Bokeh.embed.embed_items(docs_json, render_items); | ||
| } | ||
| if (root.Bokeh !== undefined) { | ||
| embed_document(root); | ||
| } else { | ||
| let attempts = 0; | ||
| const timer = setInterval(function(root) { | ||
| if (root.Bokeh !== undefined) { | ||
| clearInterval(timer); | ||
| embed_document(root); | ||
| } else { | ||
| attempts++; | ||
| if (attempts > 100) { | ||
| clearInterval(timer); | ||
| console.log("Bokeh: ERROR: Unable to run BokehJS code because BokehJS library is missing"); | ||
| } | ||
| } | ||
| }, 10, root) | ||
| } | ||
| })(window); | ||
| }); | ||
| }; | ||
| if (document.readyState != "loading") fn(); | ||
| else document.addEventListener("DOMContentLoaded", fn); | ||
| })(); | ||
| </script> | ||
| </body> | ||
| </html> No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for native_id references in Bokeh snapshot files
echo "=== Files containing native_id ==="
find test/__snapshots__ -name "*.raw" -type f -exec grep -l "native_id" {} \; 2>/dev/null
echo -e "\n=== Checking this specific snapshot for native_id in data ==="
file_path="test/__snapshots__/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs0].raw"
if [ -f "$file_path" ]; then
grep -o '"data":{[^}]*}' "$file_path" | grep -q "native_id" && echo "native_id found in data" || echo "native_id NOT found in data"
# Check HoverTool references
grep -o '"tooltips":\[\[.*\]\]' "$file_path"
else
echo "File not found at: $file_path"
fi
echo -e "\n=== Checking other spectrum Bokeh snapshots ==="
ls -la test/__snapshots__/test_spectrum/*bokeh*.raw 2>/dev/null | wc -lRepository: OpenMS/pyopenms_viz
Length of output: 1328
🏁 Script executed:
#!/bin/bash
# Check if ANY of the snapshot files have native_id in their ColumnDataSource data
echo "=== Checking if native_id appears in data within any snapshot ==="
for file in test/__snapshots__/test_spectrum/test_spectrum_plot[ms_bokeh-kwargs0].raw test/__snapshots__/test_spectrum/test_mirror_spectrum[ms_bokeh].raw; do
if [ -f "$file" ]; then
echo "File: $(basename $file)"
# Extract the data entries section and check for native_id
grep -o '"data":{[^}]*}' "$file" | head -c 500
echo ""
if grep -o '"data":{[^}]*}' "$file" | grep -q "native_id"; then
echo "✓ native_id found in data"
else
echo "✗ native_id NOT found in data"
fi
echo ""
fi
done
echo "=== Summary: Checking all 15 Bokeh snapshots for native_id in data ==="
count=0
for file in test/__snapshots__/test_spectrum/*bokeh*.raw; do
if grep -o '"data":{[^}]*}' "$file" 2>/dev/null | grep -q "native_id"; then
((count++))
fi
done
echo "Snapshots with native_id in data: $count / 15"Repository: OpenMS/pyopenms_viz
Length of output: 1106
🏁 Script executed:
#!/bin/bash
# Find the source code that generates these Bokeh visualizations
echo "=== Finding Python files that generate Bokeh plots ==="
find . -name "*.py" -type f | xargs grep -l "native_id" 2>/dev/null | grep -v __pycache__ | head -20
echo -e "\n=== Check if native_id is part of the spectrum data structure ==="
find . -name "*.py" -type f | xargs grep -l "native.id\|native_id" 2>/dev/null | grep -v __pycache__ | grep -v test
echo -e "\n=== Look for HoverTool configuration with native_id ==="
find . -name "*.py" -type f | xargs grep -B5 -A5 "native_id" 2>/dev/null | head -40Repository: OpenMS/pyopenms_viz
Length of output: 2440
🏁 Script executed:
#!/bin/bash
# Find the test file that generates this snapshot
echo "=== Finding test file for spectrum_binning ==="
find . -name "*.py" -path "*/test*" -type f | xargs grep -l "test_spectrum_binning" 2>/dev/null | head -5
# Look at the test file to understand the test data
echo -e "\n=== Examining test file structure ==="
test_file=$(find . -name "*.py" -path "*/test*" -type f | xargs grep -l "test_spectrum_binning" 2>/dev/null | head -1)
if [ -n "$test_file" ]; then
echo "Test file: $test_file"
wc -l "$test_file"
fi
# Check what the actual test data contains
echo -e "\n=== Looking for spectrum test data initialization ==="
find . -name "*.py" -path "*/test*" -type f | xargs grep -A10 "def test_spectrum" 2>/dev/null | head -60Repository: OpenMS/pyopenms_viz
Length of output: 2868
🏁 Script executed:
#!/bin/bash
# Look at the test file to find the spectrum_data fixture
echo "=== Finding spectrum_data fixture ==="
grep -n "spectrum_data\|@pytest.fixture" ./test/test_spectrum.py | head -30
echo -e "\n=== Examining full test file ==="
cat -n ./test/test_spectrum.py | head -60
echo -e "\n=== Looking for conftest.py with fixtures ==="
find ./test -name "conftest.py" -type f | xargs cat 2>/dev/nullRepository: OpenMS/pyopenms_viz
Length of output: 5966
🏁 Script executed:
#!/bin/bash
# Check the actual test data file
echo "=== Looking for TestSpectrumDf.tsv ==="
find ./test -name "TestSpectrumDf.tsv" -type f
echo -e "\n=== Reading the test data file ==="
test_data_file=$(find ./test -name "TestSpectrumDf.tsv" -type f)
if [ -f "$test_data_file" ]; then
echo "File: $test_data_file"
head -5 "$test_data_file"
echo "..."
echo "Columns:"
head -1 "$test_data_file" | tr '\t' '\n' | nl
fi
echo -e "\n=== Checking _core.py to see how native_id is handled ==="
grep -B5 -A15 'entries = {"m/z": x, "intensity": y}' ./pyopenms_viz/_core.pyRepository: OpenMS/pyopenms_viz
Length of output: 1691
🏁 Script executed:
#!/bin/bash
# Check the columns in the test data properly
echo "=== Test data columns ==="
head -1 ./test/test_data/TestSpectrumDf.tsv | tr '\t' '\n' | cat -n
# Now check how Bokeh backend creates the ColumnDataSource
echo -e "\n=== Searching for Bokeh ColumnDataSource creation ==="
find ./pyopenms_viz -name "*.py" -type f | xargs grep -l "ColumnDataSource" 2>/dev/null
echo -e "\n=== Checking Bokeh backend code ==="
find ./pyopenms_viz -name "*bokeh*" -type f
echo -e "\n=== Looking at bokeh implementation ==="
find ./pyopenms_viz -name "*.py" | xargs grep -B3 -A10 "ColumnDataSource\|custom_hover_data" 2>/dev/null | head -80Repository: OpenMS/pyopenms_viz
Length of output: 4878
🏁 Script executed:
#!/bin/bash
# Now let's look at how the Bokeh backend builds the ColumnDataSource data
echo "=== Full bokeh core.py to understand data source creation ==="
wc -l ./pyopenms_viz/_bokeh/core.py
echo -e "\n=== Looking for where plot() method adds data ==="
grep -n "def plot\|ColumnDataSource\|self.fig.line\|source=" ./pyopenms_viz/_bokeh/core.py | head -30
echo -e "\n=== Searching for where custom_hover_data is used ==="
grep -B10 -A10 "custom_hover_data" ./pyopenms_viz/_bokeh/core.py
echo -e "\n=== Check spectrum-specific implementation ==="
find ./pyopenms_viz/_bokeh -name "*.py" | xargs grep -l "spectrum"Repository: OpenMS/pyopenms_viz
Length of output: 3951
🏁 Script executed:
#!/bin/bash
# Let's look at the spectrum-specific implementation more carefully
echo "=== Spectrum implementation in bokeh ==="
grep -n "class.*Spectrum\|def plot" ./pyopenms_viz/_bokeh/core.py | head -20
echo -e "\n=== Looking at how self.data is constructed in spectrum ==="
sed -n '289,350p' ./pyopenms_viz/_bokeh/core.py
echo -e "\n=== Let's look at the parent class to understand data flow ==="
grep -n "class.*Spectrum" ./pyopenms_viz/_core.py
echo -e "\n=== Checking if there's any column dropping in spectrum ==="
grep -B5 -A15 "class.*Spectrum" ./pyopenms_viz/_core.py | head -80Repository: OpenMS/pyopenms_viz
Length of output: 3364
🏁 Script executed:
#!/bin/bash
# Let's trace how custom_hover_data should be used
echo "=== Checking how custom_hover_data is passed to ColumnDataSource ==="
grep -n "custom_hover_data" ./pyopenms_viz/_bokeh/core.py
echo -e "\n=== Checking the base class to understand custom_hover_data intent ==="
grep -B10 -A10 "def _create_tooltips" ./pyopenms_viz/_core.py
echo -e "\n=== Looking at how other backends use custom_hover_data ==="
find ./pyopenms_viz -name "*.py" | xargs grep -n "custom_hover_data" 2>/dev/null | grep -v "^Binary"Repository: OpenMS/pyopenms_viz
Length of output: 5433
🏁 Script executed:
#!/bin/bash
# Key question: Is native_id being filtered out of self.data before Bokeh plotting?
echo "=== Checking if there's any column filtering in spectrum plot ==="
grep -B20 -A30 "class BOKEHSpectrumPlot" ./pyopenms_viz/_bokeh/core.py
echo -e "\n=== Let's check if self.data is modified before ColumnDataSource ==="
sed -n '538,650p' ./pyopenms_viz/_bokeh/core.py
echo -e "\n=== Check if Bokeh's ColumnDataSource should auto-include all dataframe columns ==="
grep -B5 -A15 "def plot.*:" ./pyopenms_viz/_bokeh/core.py | head -60Repository: OpenMS/pyopenms_viz
Length of output: 7317
🏁 Script executed:
#!/bin/bash
# Let's check what _create_tooltips returns and how it's supposed to be used
echo "=== Full _create_tooltips implementation ==="
grep -B5 -A50 "def _create_tooltips" ./pyopenms_viz/_core.py | head -100
echo -e "\n=== Checking how Plotly uses custom_hover_data ==="
sed -n '142,180p' ./pyopenms_viz/_plotly/core.py
echo -e "\n=== Checking if Bokeh should use custom_hover_data but doesn't ==="
sed -n '107,120p' ./pyopenms_viz/_bokeh/core.pyRepository: OpenMS/pyopenms_viz
Length of output: 4151
🏁 Script executed:
#!/bin/bash
# Key insight: Bokeh creates ColumnDataSource from self.data, which should include all columns
# Let's verify if self.data actually contains all columns including native_id
echo "=== Checking spectrum plot flow in _core.py ==="
sed -n '770,810p' ./pyopenms_viz/_core.py
echo -e "\n=== What data is passed to the plot ==="
grep -B20 "spectrumPlot.generate" ./pyopenms_viz/_core.py | head -40
echo -e "\n=== Check if native_id gets dropped somewhere ==="
grep -n "drop\|select\|columns" ./pyopenms_viz/_core.py | grep -i spectrumRepository: OpenMS/pyopenms_viz
Length of output: 3050
🏁 Script executed:
#!/bin/bash
# The key is convert_for_line_plots - let's check what it does
echo "=== Finding convert_for_line_plots method ==="
grep -n "def convert_for_line_plots" ./pyopenms_viz/_core.py
echo -e "\n=== Full convert_for_line_plots implementation ==="
grep -B3 -A30 "def convert_for_line_plots" ./pyopenms_viz/_core.py
echo -e "\n=== Check if this method only keeps x and y columns ==="
sed -n '648,710p' ./pyopenms_viz/_core.pyRepository: OpenMS/pyopenms_viz
Length of output: 3781
🏁 Script executed:
#!/bin/bash
# Let's confirm that native_id SHOULD be included in the tooltip but the data pipeline removes it
echo "=== Checking the tooltip creation for spectrum ==="
sed -n '1090,1115p' ./pyopenms_viz/_core.py
echo -e "\n=== Verify: native_id is explicitly checked but data was stripped ==="
grep -B2 "native_id" ./pyopenms_viz/_core.py | head -20Repository: OpenMS/pyopenms_viz
Length of output: 1509
The HoverTool tooltip references @native_id but the data will be missing from ColumnDataSource.
The test data includes a native_id column, and the spectrum plotting code explicitly checks for and includes it in tooltips (if present). However, convert_for_line_plots() strips all columns except x, y, and the grouping column before creating the Bokeh ColumnDataSource, removing native_id and other metadata columns. This causes the tooltip to reference a field that won't exist in the data, resulting in empty hover values for "native id".
This pattern affects all 15 Bokeh spectrum snapshots. The fix requires either:
- Preserving metadata columns (like
native_id) through the line plot conversion, or - Removing
native_idfrom the tooltip entries if it won't be available in the final data source
🤖 Prompt for AI Agents
In `@test/__snapshots__/test_spectrum/test_spectrum_binning`[ms_bokeh-kwargs0].raw
around lines 1 - 61, convert_for_line_plots() is currently stripping all columns
except x, y and group before building the Bokeh ColumnDataSource, which removes
metadata like native_id referenced by the HoverTool tooltips; update
convert_for_line_plots() (or the helper that builds the ColumnDataSource) to
preserve any non-index metadata columns (e.g., native_id) when present so the
ColumnDataSource contains x, y, group plus metadata fields, or alternatively
remove native_id from the HoverTool tooltips construction (where HoverTool is
created) so tooltips only reference fields guaranteed to exist.
| "x": { | ||
| "dtype": "f8", | ||
| "bdata": "O99PjZd+SUA730+Nl35JQDvfT42XfklAY3\u002fZPXmIUkBjf9k9eYhSQGN\u002f2T15iFJAio7k8h8iWUCKjuTyHyJZQIqO5PIfIllAexSuR+FiWUB7FK5H4WJZQHsUrkfhYllALpCg+DGiWUAukKD4MaJZQC6QoPgxollAZmZmZmYGZEBmZmZmZgZkQGZmZmZmBmRAsp3vp8b\u002fZUCyne+nxv9lQLKd76fG\u002f2VAWvW52ooNaUBa9bnaig1pQFr1udqKDWlAWvW52oodaUBa9bnaih1pQFr1udqKHWlA" | ||
| }, | ||
| "y": { | ||
| "dtype": "i1", | ||
| "bdata": "AAoAABQAABkAAAwAAAYAAA0AAAgAABEAAAcA" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
python3 << 'EOF'
import base64
import struct
# From the snapshot
x_bdata = "O99PjZd+SUA730+Nl35JQDvfT42XfklAY3/ZPXmIUkBjf9k9eYhSQGN/2T15iFJAio7k8h8iWUCKjuTyHyJZQIqO5PIfIllAexSuR+FiWUB7FK5H4WJZQHsUrkfhYllALpCg+DGiWUAukKD4MaJZQC6QoPgxollAZmZmZmYGZEBmZmZmZgZkQGZmZmZmBmRAsp3vp8b/ZUCyne+nxv9lQLKd76fG/2VAWvW52ooNaUBa9bnaig1pQFr1udqKDWlAWvW52oodaUBa9bnaih1pQFr1udqKHWlA"
y_bdata = "AAoAABQAABkAAAwAAAYAAA0AAAgAABEAAAcA"
# Decode x data (float64 = 8 bytes each)
x_bytes = base64.b64decode(x_bdata)
x_count = len(x_bytes) // 8
print(f"x data points: {x_count}")
# Decode y data (int8 = 1 byte each)
y_bytes = base64.b64decode(y_bdata)
y_count = len(y_bytes)
print(f"y data bytes: {y_count}")
# Print decoded y values to see actual content
y_values = struct.unpack('b' * y_count, y_bytes)
print(f"y values: {y_values}")
print(f"\nExpected customdata entries: 9")
print(f"Match: {x_count == 9 and y_count == 9}")
EOFRepository: OpenMS/pyopenms_viz
Length of output: 149
🏁 Script executed:
# Check if the snapshot file exists and get its context
find . -name "test_spectrum_binning*.raw" -type f | head -20Repository: OpenMS/pyopenms_viz
Length of output: 1277
🏁 Script executed:
# Read the snapshot file to examine the actual structure
head -n 100 "./test/__snapshots__/test_spectrum/test_spectrum_binning[ms_plotly-kwargs0].raw"Repository: OpenMS/pyopenms_viz
Length of output: 2288
🏁 Script executed:
# Extract just the x and y bdata values and decode them
python3 << 'EOFINLINE'
import base64
import json
# Read the snapshot file
with open("./test/__snapshots__/test_spectrum/test_spectrum_binning[ms_plotly-kwargs0].raw", "r") as f:
content = f.read()
# Find the x and y data sections
# Extract bdata strings
import re
x_match = re.search(r'"x":\s*\{\s*"dtype":\s*"f8",\s*"bdata":\s*"([^"]+)"', content)
y_match = re.search(r'"y":\s*\{\s*"dtype":\s*"i1",\s*"bdata":\s*"([^"]+)"', content)
if x_match and y_match:
x_bdata = x_match.group(1)
y_bdata = y_match.group(1)
# Decode
x_bytes = base64.b64decode(x_bdata)
y_bytes = base64.b64decode(y_bdata)
x_count = len(x_bytes) // 8 # float64
y_count = len(y_bytes) # int8
print(f"x data points (float64): {x_count}")
print(f"y data points (int8): {y_count}")
print(f"Data aligned: {x_count == y_count}")
else:
print("Could not find x/y bdata in file")
EOFINLINERepository: OpenMS/pyopenms_viz
Length of output: 135
Critical: Binary-encoded x/y data is misaligned.
The x/y data in binary format does not align. Decoding reveals 28 float64 x-values but only 27 int8 y-values. Plotly requires matching array lengths for x and y coordinates to render correctly. This mismatch will cause rendering errors. Additionally, neither count (28/27) matches the 9 customdata entries, indicating a broader data structure problem that needs resolution.
🤖 Prompt for AI Agents
In
`@test/__snapshots__/test_spectrum/test_spectrum_binning`[ms_plotly-kwargs0].raw
around lines 8 - 15, The snapshot contains misaligned binary arrays: the "x"
field (dtype f8) decodes to 28 values while "y" (dtype i1) decodes to 27, and
neither matches the 9 entries in "customdata" in test_spectrum_binning; fix by
ensuring the generator producing these arrays creates matching lengths—make "x"
and "y" the same length and align them with the number of "customdata" entries
(e.g., adjust the data-producing code in the test or fixture so "x", "y" and
"customdata" all have the same count), then re-encode the bdata for "x" and "y"
to reflect the corrected arrays and update the snapshot for
test_spectrum_binning.
| @@ -12,25 +12,25 @@ | |||
| padding: 0; | |||
| } | |||
| </style> | |||
| <script type="text/javascript" src="https://cdn.bokeh.org/bokeh/release/bokeh-3.6.0.min.js"></script> | |||
| <script type="text/javascript" src="https://cdn.bokeh.org/bokeh/release/bokeh-3.4.1.min.js"></script> | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Bokeh version specified in project dependencies
echo "=== Checking pyproject.toml ==="
fd -t f "pyproject.toml" -x cat {}
echo -e "\n=== Checking requirements files ==="
fd -g "requirements*.txt" -x cat {}
echo -e "\n=== Checking setup.py/setup.cfg ==="
fd -t f \( -name "setup.py" -o -name "setup.cfg" \) -x cat {}
echo -e "\n=== Checking poetry.lock or pip.lock ==="
fd -t f \( -name "poetry.lock" -o -name "pip.lock" \) -x head -100 {}Repository: OpenMS/pyopenms_viz
Length of output: 13825
Regenerate snapshot with current Bokeh version.
The snapshot references Bokeh 3.4.1, but the project's resolved dependencies use Bokeh 3.6.0 (requirements.txt) or 3.6.2 (docs/requirements.txt). Regenerate this snapshot with the current environment to ensure it reflects the actual tested behavior.
🤖 Prompt for AI Agents
In
`@test/__snapshots__/test_spectrum/test_spectrum_plot_with_peak_color`[ms_bokeh-kwargs1].raw
at line 15, The stored snapshot references Bokeh 3.4.1 in the script tag and
must be updated to match the current test environment (Bokeh 3.6.x); regenerate
the snapshot for test_spectrum_plot_with_peak_color[ms_bokeh-kwargs1] so the
script src reflects the resolved Bokeh version (e.g., update the <script ...
bokeh-3.4.1.min.js> entry to bokeh-3.6.x.min.js) by re-running the snapshot test
or snapshot update command to produce the new expected output.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 99 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyopenms_viz/testing/MatplotlibSnapshotExtension.py (1)
40-60: Update serialize docstring and silence unusedkwargs.The return type is now bytes and Ruff flags
kwargsas unused.📝 Suggested fix
- def serialize(self, data: SerializableData, **kwargs: Any) -> bytes: + def serialize(self, data: SerializableData, **kwargs: Any) -> bytes: # noqa: ARG002 @@ - Returns: - str: Image object + Returns: + bytes: PNG bytes
🤖 Fix all issues with AI agents
In `@pyopenms_viz/testing/BokehSnapshotExtension.py`:
- Line 142: The serialize method is currently leaving **kwargs unused which
triggers Ruff ARG002; update the method signature in BokehSnapshotExtension.py
(serialize(self, data: SerializableData, **kwargs: Any) -> bytes) to silence the
warning by marking the var as intentionally unused—e.g., change **kwargs to
**_kwargs or explicitly reference kwargs in the body with a no-op assignment
like _ = kwargs or add a noqa ARG002 comment—so the signature stays compatible
but Ruff no longer flags it.
In `@requirements-dev.txt`:
- Around line 1-2: Update the pytest requirement to ensure compatibility with
syrupy>=5.0.0 by changing the pytest entry in requirements-dev.txt from "pytest"
to "pytest>=8"; this guarantees pytest version >=8 for the "pytest" dependency
to satisfy "syrupy>=5.0.0".
In `@test/__snapshots__/test_spectrum/test_spectrum_plot`[ms_bokeh-kwargs0].html:
- Line 15: The snapshot HTML contains a hardcoded script tag referencing
bokeh-3.4.1; update the test snapshot for
test_spectrum_plot[ms_bokeh-kwargs0].html to reference the project's pinned
bokeh version (bokeh-3.6.0) by regenerating the snapshot (re-run the snapshot
generation/test that produces this file) so the embedded script src becomes
https://cdn.bokeh.org/bokeh/release/bokeh-3.6.0.min.js and the test artifacts
match requirements.txt.
In `@test/__snapshots__/test_spectrum/test_spectrum_plot`[ms_bokeh-kwargs2].html:
- Line 15: The snapshot HTML references "bokeh-3.4.1.min.js" but locked deps
require bokeh 3.6.x; re-generate the failing snapshot file (the snapshot named
test_spectrum_plot[ms_bokeh-kwargs2].html that contains the script tag
"bokeh-3.4.1.min.js") in an environment with the pinned bokeh version (install
bokeh==3.6.0 or 3.6.2 per requirements.txt / docs/requirements.txt), run the
test that creates the Bokeh output to update the snapshot (so the script tag
becomes the correct bokeh-3.6.x URL), and commit the regenerated snapshot.
Ensure your test environment uses the same dependency resolution as CI to avoid
future mismatches.
♻️ Duplicate comments (2)
test/__snapshots__/test_spectrum/test_spectrum_binning[ms_plotly-kwargs0].json (1)
8-15: Verify binary-encoded x/y data alignment.A previous review flagged potential misalignment between x (float64) and y (int8) binary arrays. The customdata array now contains 27 entries (9 unique peaks × 3 points each for stick-plot rendering). Please verify that both x and y decode to exactly 27 values to match.
#!/bin/bash # Verify binary data alignment in the snapshot python3 << 'EOF' import base64 import json # Read the snapshot file with open("test/__snapshots__/test_spectrum/test_spectrum_binning[ms_plotly-kwargs0].json", "r") as f: data = json.load(f) trace = data["data"][0] # Decode x (float64 = 8 bytes each) x_bdata = trace["x"]["bdata"] x_bytes = base64.b64decode(x_bdata) x_count = len(x_bytes) // 8 # Decode y (int8 = 1 byte each) y_bdata = trace["y"]["bdata"] y_bytes = base64.b64decode(y_bdata) y_count = len(y_bytes) # Count customdata entries customdata_count = len(trace["customdata"]) print(f"x data points (float64): {x_count}") print(f"y data points (int8): {y_count}") print(f"customdata entries: {customdata_count}") print(f"\nAll arrays aligned: {x_count == y_count == customdata_count}") if x_count != y_count or x_count != customdata_count: print("\n⚠️ MISMATCH DETECTED - Arrays must have equal lengths for Plotly to render correctly") else: print("\n✓ Data alignment verified") EOFtest/__snapshots__/test_spectrum/test_mirror_spectrum[ms_plotly].json (1)
566-577: LGTM on the scattermap template addition.Consistent with the other Plotly snapshot updates in this PR.
🧹 Nitpick comments (3)
requirements.txt (1)
269-270: Consider re-freezing the pip-compiled requirements.Line 269 leaves syrupy as a range in an auto-generated lockfile, which can make environments non-reproducible. If unintentional, re-run
pip-compileso this file pins an exact version (or update the header if ranges are intentional).pyopenms_viz/testing/PlotlySnapshotExtension.py (1)
47-68: Consider removing debug print statements.The
compare_jsonmethod contains multipleprint()statements that output comparison details. While useful for debugging, these may clutter test output in CI/CD pipelines. Consider using logging at DEBUG level or removing them entirely if not needed for diagnostics.♻️ Suggested refactor using logging
+import logging + +logger = logging.getLogger(__name__) + class PlotlySnapshotExtension(SingleFileSnapshotExtension): ... `@staticmethod` def compare_json(json1, json2) -> bool: ... if isinstance(json1, dict) and isinstance(json2, dict): for key in json1.keys(): if key not in json2: - print(f"Key {key} not in second json") + logger.debug(f"Key {key} not in second json") return False if not PlotlySnapshotExtension.compare_json(json1[key], json2[key]): - print(f"Values for key {key} not equal") + logger.debug(f"Values for key {key} not equal") return False return True elif isinstance(json1, list) and isinstance(json2, list): if len(json1) != len(json2): - print("Lists have different lengths") + logger.debug("Lists have different lengths") return False ... else: if isinstance(json1, float): if not math.isclose(json1, json2): - print(f"Values not equal: {json1} != {json2}") + logger.debug(f"Values not equal: {json1} != {json2}") return False else: if json1 != json2: - print(f"Values not equal: {json1} != {json2}") + logger.debug(f"Values not equal: {json1} != {json2}") return False return Truepyopenms_viz/testing/MatplotlibSnapshotExtension.py (1)
19-38: Avoid broadExceptionwhen decoding PNG bytes.Catching all exceptions can mask corrupted PNGs; prefer explicit Image type checks to fail fast on invalid bytes.
♻️ Suggested refactor
- try: - serialized_img = Image.open(BytesIO(serialized_data)) - except Exception: - # If already an Image object, use it directly - serialized_img = serialized_data + if isinstance(serialized_data, Image.Image): + serialized_img = serialized_data + else: + serialized_img = Image.open(BytesIO(serialized_data)) - try: - snapshot_img = Image.open(BytesIO(snapshot_data)) - except Exception: - snapshot_img = snapshot_data + if isinstance(snapshot_data, Image.Image): + snapshot_img = snapshot_data + else: + snapshot_img = Image.open(BytesIO(snapshot_data))
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (22)
test/__snapshots__/test_chromatogram/test_chromatogram_plot[ms_matplotlib-kwargs0].pngis excluded by!**/*.pngtest/__snapshots__/test_chromatogram/test_chromatogram_plot[ms_matplotlib-kwargs3].pngis excluded by!**/*.pngtest/__snapshots__/test_chromatogram/test_chromatogram_plot[ms_matplotlib-kwargs4].pngis excluded by!**/*.pngtest/__snapshots__/test_mobilogram/test_mobilogram_plot[ms_matplotlib-kwargs0].pngis excluded by!**/*.pngtest/__snapshots__/test_mobilogram/test_mobilogram_plot[ms_matplotlib-kwargs3].pngis excluded by!**/*.pngtest/__snapshots__/test_peakmap3d/test_peakmap_plot[ms_matplotlib-kwargs0].pngis excluded by!**/*.pngtest/__snapshots__/test_peakmap3d/test_peakmap_plot[ms_matplotlib-kwargs2].pngis excluded by!**/*.pngtest/__snapshots__/test_spectrum/test_mirror_spectrum[ms_matplotlib].pngis excluded by!**/*.pngtest/__snapshots__/test_spectrum/test_spectrum_binning[ms_matplotlib-kwargs0].pngis excluded by!**/*.pngtest/__snapshots__/test_spectrum/test_spectrum_binning[ms_matplotlib-kwargs1].pngis excluded by!**/*.pngtest/__snapshots__/test_spectrum/test_spectrum_binning[ms_matplotlib-kwargs2].pngis excluded by!**/*.pngtest/__snapshots__/test_spectrum/test_spectrum_binning[ms_matplotlib-kwargs3].pngis excluded by!**/*.pngtest/__snapshots__/test_spectrum/test_spectrum_binning[ms_matplotlib-kwargs4].pngis excluded by!**/*.pngtest/__snapshots__/test_spectrum/test_spectrum_plot[ms_matplotlib-kwargs0].pngis excluded by!**/*.pngtest/__snapshots__/test_spectrum/test_spectrum_plot[ms_matplotlib-kwargs1].pngis excluded by!**/*.pngtest/__snapshots__/test_spectrum/test_spectrum_plot[ms_matplotlib-kwargs2].pngis excluded by!**/*.pngtest/__snapshots__/test_spectrum/test_spectrum_plot[ms_matplotlib-kwargs3].pngis excluded by!**/*.pngtest/__snapshots__/test_spectrum/test_spectrum_plot_with_peak_color[ms_matplotlib-kwargs0].pngis excluded by!**/*.pngtest/__snapshots__/test_spectrum/test_spectrum_plot_with_peak_color[ms_matplotlib-kwargs1].pngis excluded by!**/*.pngtest/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_matplotlib-kwargs0].pngis excluded by!**/*.pngtest/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_matplotlib-kwargs1].pngis excluded by!**/*.pngtest/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_matplotlib-kwargs2].pngis excluded by!**/*.png
📒 Files selected for processing (75)
pyopenms_viz/testing/BokehSnapshotExtension.pypyopenms_viz/testing/MatplotlibSnapshotExtension.pypyopenms_viz/testing/PlotlySnapshotExtension.pypyproject.tomlrequirements-dev.txtrequirements.txttest/__snapshots__/test_chromatogram/test_chromatogram_plot[ms_bokeh-kwargs0].htmltest/__snapshots__/test_chromatogram/test_chromatogram_plot[ms_bokeh-kwargs1].htmltest/__snapshots__/test_chromatogram/test_chromatogram_plot[ms_bokeh-kwargs2].htmltest/__snapshots__/test_chromatogram/test_chromatogram_plot[ms_bokeh-kwargs3].htmltest/__snapshots__/test_chromatogram/test_chromatogram_plot[ms_bokeh-kwargs4].htmltest/__snapshots__/test_chromatogram/test_chromatogram_plot[ms_plotly-kwargs0].jsontest/__snapshots__/test_chromatogram/test_chromatogram_plot[ms_plotly-kwargs1].jsontest/__snapshots__/test_chromatogram/test_chromatogram_plot[ms_plotly-kwargs2].jsontest/__snapshots__/test_chromatogram/test_chromatogram_plot[ms_plotly-kwargs3].jsontest/__snapshots__/test_chromatogram/test_chromatogram_plot[ms_plotly-kwargs4].jsontest/__snapshots__/test_chromatogram/test_chromatogram_with_annotation[ms_bokeh-kwargs0].htmltest/__snapshots__/test_chromatogram/test_chromatogram_with_annotation[ms_plotly-kwargs0].jsontest/__snapshots__/test_mobilogram/test_mobilogram_plot[ms_bokeh-kwargs0].htmltest/__snapshots__/test_mobilogram/test_mobilogram_plot[ms_bokeh-kwargs1].htmltest/__snapshots__/test_mobilogram/test_mobilogram_plot[ms_bokeh-kwargs2].htmltest/__snapshots__/test_mobilogram/test_mobilogram_plot[ms_bokeh-kwargs3].htmltest/__snapshots__/test_mobilogram/test_mobilogram_plot[ms_plotly-kwargs0].jsontest/__snapshots__/test_mobilogram/test_mobilogram_plot[ms_plotly-kwargs1].jsontest/__snapshots__/test_mobilogram/test_mobilogram_plot[ms_plotly-kwargs2].jsontest/__snapshots__/test_mobilogram/test_mobilogram_plot[ms_plotly-kwargs3].jsontest/__snapshots__/test_peakmap/test_peakmap_mz_im[ms_bokeh].htmltest/__snapshots__/test_peakmap/test_peakmap_mz_im[ms_plotly].jsontest/__snapshots__/test_peakmap/test_peakmap_plot[ms_bokeh-kwargs0].htmltest/__snapshots__/test_peakmap/test_peakmap_plot[ms_bokeh-kwargs1].htmltest/__snapshots__/test_peakmap/test_peakmap_plot[ms_bokeh-kwargs2].htmltest/__snapshots__/test_peakmap/test_peakmap_plot[ms_bokeh-kwargs3].htmltest/__snapshots__/test_peakmap/test_peakmap_plot[ms_bokeh-kwargs4].htmltest/__snapshots__/test_peakmap/test_peakmap_plot[ms_plotly-kwargs0].jsontest/__snapshots__/test_peakmap/test_peakmap_plot[ms_plotly-kwargs1].jsontest/__snapshots__/test_peakmap/test_peakmap_plot[ms_plotly-kwargs2].jsontest/__snapshots__/test_peakmap/test_peakmap_plot[ms_plotly-kwargs3].jsontest/__snapshots__/test_peakmap/test_peakmap_plot[ms_plotly-kwargs4].jsontest/__snapshots__/test_peakmap3d/test_peakmap_plot[ms_plotly-kwargs0].jsontest/__snapshots__/test_peakmap3d/test_peakmap_plot[ms_plotly-kwargs1].jsontest/__snapshots__/test_peakmap3d/test_peakmap_plot[ms_plotly-kwargs2].jsontest/__snapshots__/test_peakmap_marginals/test_peakmap_marginals[ms_bokeh].htmltest/__snapshots__/test_peakmap_marginals/test_peakmap_marginals[ms_plotly].jsontest/__snapshots__/test_peakmap_marginals/test_peakmap_mz_im[ms_bokeh].htmltest/__snapshots__/test_peakmap_marginals/test_peakmap_mz_im[ms_plotly].jsontest/__snapshots__/test_spectrum/test_mirror_spectrum[ms_bokeh].htmltest/__snapshots__/test_spectrum/test_mirror_spectrum[ms_plotly].jsontest/__snapshots__/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs0].htmltest/__snapshots__/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs1].htmltest/__snapshots__/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs2].htmltest/__snapshots__/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs3].htmltest/__snapshots__/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs4].htmltest/__snapshots__/test_spectrum/test_spectrum_binning[ms_plotly-kwargs0].jsontest/__snapshots__/test_spectrum/test_spectrum_binning[ms_plotly-kwargs1].jsontest/__snapshots__/test_spectrum/test_spectrum_binning[ms_plotly-kwargs2].jsontest/__snapshots__/test_spectrum/test_spectrum_binning[ms_plotly-kwargs3].jsontest/__snapshots__/test_spectrum/test_spectrum_binning[ms_plotly-kwargs4].jsontest/__snapshots__/test_spectrum/test_spectrum_plot[ms_bokeh-kwargs0].htmltest/__snapshots__/test_spectrum/test_spectrum_plot[ms_bokeh-kwargs1].htmltest/__snapshots__/test_spectrum/test_spectrum_plot[ms_bokeh-kwargs2].htmltest/__snapshots__/test_spectrum/test_spectrum_plot[ms_bokeh-kwargs3].htmltest/__snapshots__/test_spectrum/test_spectrum_plot[ms_plotly-kwargs0].jsontest/__snapshots__/test_spectrum/test_spectrum_plot[ms_plotly-kwargs1].jsontest/__snapshots__/test_spectrum/test_spectrum_plot[ms_plotly-kwargs2].jsontest/__snapshots__/test_spectrum/test_spectrum_plot[ms_plotly-kwargs3].jsontest/__snapshots__/test_spectrum/test_spectrum_plot_with_peak_color[ms_bokeh-kwargs0].htmltest/__snapshots__/test_spectrum/test_spectrum_plot_with_peak_color[ms_bokeh-kwargs1].htmltest/__snapshots__/test_spectrum/test_spectrum_plot_with_peak_color[ms_plotly-kwargs0].jsontest/__snapshots__/test_spectrum/test_spectrum_plot_with_peak_color[ms_plotly-kwargs1].jsontest/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_bokeh-kwargs0].htmltest/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_bokeh-kwargs1].htmltest/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_bokeh-kwargs2].htmltest/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_plotly-kwargs0].jsontest/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_plotly-kwargs1].jsontest/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_plotly-kwargs2].json
🚧 Files skipped from review as they are similar to previous changes (3)
- test/snapshots/test_spectrum/test_spectrum_with_annotations[ms_bokeh-kwargs2].html
- test/snapshots/test_spectrum/test_spectrum_plot[ms_bokeh-kwargs1].html
- test/snapshots/test_spectrum/test_spectrum_plot[ms_bokeh-kwargs3].html
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-16T14:23:00.017Z
Learnt from: DimitriPapadopoulos
Repo: OpenMS/pyopenms_viz PR: 137
File: pyproject.toml:7-10
Timestamp: 2026-01-16T14:23:00.017Z
Learning: When reviewing pyproject.toml files that use PEP 621, the authors array may contain entries with only a name field, only an email field, or both fields, and multiple separate entries are valid. Do not flag consolidation of distinct entries unless there is an explicit intention to merge authors. This guideline applies to all pyproject.toml files in the repository that follow PEP 621 authors structure.
Applied to files:
pyproject.toml
🧬 Code graph analysis (2)
pyopenms_viz/testing/BokehSnapshotExtension.py (4)
pyopenms_viz/testing/MatplotlibSnapshotExtension.py (1)
serialize(40-60)pyopenms_viz/testing/PlotlySnapshotExtension.py (1)
serialize(72-82)pyopenms_viz/testing/NumpySnapshotExtension.py (1)
serialize(59-60)pyopenms_viz/testing/PandasSnapshotExtension.py (1)
serialize(54-55)
pyopenms_viz/testing/PlotlySnapshotExtension.py (4)
pyopenms_viz/testing/BokehSnapshotExtension.py (2)
matches(46-66)serialize(142-152)pyopenms_viz/testing/MatplotlibSnapshotExtension.py (2)
matches(19-38)serialize(40-60)pyopenms_viz/testing/NumpySnapshotExtension.py (2)
matches(21-36)serialize(59-60)pyopenms_viz/testing/PandasSnapshotExtension.py (2)
matches(21-32)serialize(54-55)
🪛 Ruff (0.14.11)
pyopenms_viz/testing/BokehSnapshotExtension.py
142-142: Unused method argument: kwargs
(ARG002)
pyopenms_viz/testing/PlotlySnapshotExtension.py
72-72: Unused method argument: kwargs
(ARG002)
pyopenms_viz/testing/MatplotlibSnapshotExtension.py
23-23: Do not catch blind exception: Exception
(BLE001)
29-29: Do not catch blind exception: Exception
(BLE001)
40-40: Unused method argument: kwargs
(ARG002)
⏰ 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). (1)
- GitHub Check: CodeQL analysis (python)
🔇 Additional comments (70)
test/__snapshots__/test_peakmap/test_peakmap_plot[ms_bokeh-kwargs0].html (1)
15-33: Snapshot update only — no review comments.test/__snapshots__/test_spectrum/test_spectrum_binning[ms_plotly-kwargs2].json (2)
8-15: Snapshot binary encoding update looks fine.
This aligns with the new snapshot serialization format.
461-472: Plotly scattermap template addition looks good.
No concerns with the template expansion in snapshots.test/__snapshots__/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs4].html (1)
15-15: Snapshot regeneration looks consistent.No issues noticed in the updated snapshot content.
Also applies to: 21-24, 32-33
test/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_bokeh-kwargs0].html (2)
15-15: Verify the Bokeh version downgrade is intentional.The Bokeh version appears to have changed from 3.6.0 to 3.4.1 based on the AI summary. Please confirm this downgrade is expected—perhaps due to a dependency pin or compatibility requirement—rather than an unintended regression in the test environment.
21-33: Snapshot IDs regenerated as expected.The UUID-based element IDs and JSON document references have been updated, which is standard behavior when regenerating test snapshots. The Bokeh document structure appears valid with proper spectrum renderers, annotations (Labels), legend items, and HoverTool configuration.
Note: This is a Bokeh backend snapshot, but the PR title references a Plotly annotation fix. Please confirm this snapshot regeneration is expected as part of the changes (e.g., due to syrupy dependency update or test infrastructure changes).
test/__snapshots__/test_peakmap/test_peakmap_plot[ms_plotly-kwargs1].json (2)
7-67: Binary-encoded array updates look consistent.The snapshot serialization changes for marker colors and x/y arrays appear coherent across traces.
Also applies to: 221-281, 435-495, 649-709, 863-923, 1077-1137, 1291-1351
1896-1907: Template expansion looks fine.The added template entry aligns with the other trace defaults in the layout.
test/__snapshots__/test_spectrum/test_spectrum_binning[ms_plotly-kwargs0].json (2)
17-152: Customdata structure is appropriate for stick plot rendering.The triplet pattern (each unique peak repeated 3 times) correctly supports the baseline→peak→baseline line segments used in mass spectrum stick plots. The 9 unique m/z values × 3 points = 27 entries aligns with the expected data structure.
551-562: Template addition for scattermap trace type looks correct.The new
scattermapentry follows the established pattern used by other scatter-type traces in the template (consistent colorbar configuration). This appears to be a Plotly template update reflecting newer trace types.test/__snapshots__/test_spectrum/test_spectrum_plot_with_peak_color[ms_plotly-kwargs0].json (2)
9-16: Binary-encoded arrays look consistent with snapshot serialization.No issues spotted in these changes.
Also applies to: 196-203
616-627: The concern about Plotly version support forscattermapis not applicable. Plotly 5.24.1 (released September 12, 2024) bundles Plotly.js 3.0.0-rc.0, which includes full support for thescattermaptrace type (introduced in Plotly.js v2.35.0 on August 29, 2024). The pinned version specification (==5.24.1) ensures consistent behavior across environments.Likely an incorrect or invalid review comment.
test/__snapshots__/test_spectrum/test_spectrum_binning[ms_plotly-kwargs1].json (3)
8-15: Binary x/y snapshot encoding looks fine.
506-517: Template update for scattermap is OK.
17-107: customdata length aligns with decoded x/y points.
The customdata array (18 entries) matches the decoded x and y point counts (both 18), so hovertext alignment is correct.test/__snapshots__/test_peakmap3d/test_peakmap_plot[ms_plotly-kwargs2].json (1)
2475-2486: Snapshot update aligns with scattermap support.Line 2475 adds the scattermap defaults; this looks consistent with the heatmapgl → scattermap migration in Plotly templates.
pyproject.toml (1)
28-30: Confirm the testing extra remains compatible across environments.Line 29 bumps syrupy to >=5.0.0; please ensure the testing extra resolves consistently with the lockfiles and snapshot extensions.
test/__snapshots__/test_peakmap3d/test_peakmap_plot[ms_plotly-kwargs0].json (1)
2475-2486: Snapshot update aligns with scattermap support.Line 2475 adds scattermap defaults in the template data; this is consistent with the Plotly trace type update.
test/__snapshots__/test_spectrum/test_spectrum_plot[ms_bokeh-kwargs2].html (1)
21-34: LGTM on the ID updates.The element IDs, doc references, and render items are auto-generated during snapshot creation. These changes are expected when regenerating snapshots with updated test infrastructure.
test/__snapshots__/test_spectrum/test_spectrum_binning[ms_plotly-kwargs4].json (2)
8-15: LGTM on the binary data encoding.The x/y coordinate arrays now use Plotly's typed-array serialization format (
dtype: "f8"for 64-bit floats,bdatafor base64-encoded binary data). This is the expected format when using syrupy 5.0.0+ with binary serialization mode.
461-472: LGTM on the scattermap trace addition.The new
scattermaptrace type in the template data is consistent with the Plotly template structure and follows the same colorbar configuration pattern as other scatter-type traces.test/__snapshots__/test_peakmap3d/test_peakmap_plot[ms_plotly-kwargs1].json (1)
2022-2033: LGTM on the template update.The
scattermaptrace addition to the layout template is consistent with the broader PR changes across Plotly snapshots. The actual scatter3d data traces remain unchanged.test/__snapshots__/test_spectrum/test_spectrum_plot_with_peak_color[ms_bokeh-kwargs0].html (1)
15-34: LGTM on the snapshot regeneration.The changes are consistent with the other Bokeh snapshot updates (same version 3.4.1, updated auto-generated IDs). The visualization structure and data remain unchanged.
test/__snapshots__/test_spectrum/test_mirror_spectrum[ms_plotly].json (2)
8-15: LGTM on the binary data encoding.The x/y coordinate arrays correctly use Plotly's typed-array serialization format for the mirror spectrum traces.
17-152: Correct fix for hover data alignment.The customdata array now contains 3 entries per data point, matching the expanded x/y line plot format where each spectrum peak is rendered as a triplet (baseline → peak → baseline). This ensures tooltips display correctly at all points of each peak, fixing the annotation bug for single-trace spectra (issue
#88).test/__snapshots__/test_spectrum/test_spectrum_plot[ms_plotly-kwargs2].json (2)
9-16: Binary x/y snapshot update looks consistent.
The encoded x/y payloads align with the new snapshot format while keeping trace metadata intact.Also applies to: 88-95, 122-129, 171-178
609-620: Template scattermap entry looks fine.
No concerns with the added layout template entry.test/__snapshots__/test_chromatogram/test_chromatogram_with_annotation[ms_plotly-kwargs0].json (2)
9-16: Binary x/y payloads match the updated snapshot format.
No issues observed with the regenerated trace data blocks.Also applies to: 424-431, 797-804, 1219-1225, 1772-1778, 2336-2343, 2866-2872
3878-3889: Template scattermap entry looks fine.
No concerns with the added layout template entry.test/__snapshots__/test_spectrum/test_mirror_spectrum[ms_bokeh].html (1)
15-15: Snapshot-only regeneration.
No actionable review feedback for updated embed artifacts.Also applies to: 21-23, 32-33
test/__snapshots__/test_spectrum/test_spectrum_plot[ms_plotly-kwargs1].json (2)
9-16: Binary x/y snapshot update looks consistent.
The encoded payloads align with the new snapshot format.Also applies to: 58-65, 137-144
590-601: Template scattermap entry looks fine.
No concerns with the added layout template entry.test/__snapshots__/test_spectrum/test_spectrum_plot_with_peak_color[ms_bokeh-kwargs1].html (1)
15-15: Snapshot-only regeneration.
No actionable review feedback for updated embed artifacts.Also applies to: 21-23, 32-33
test/__snapshots__/test_spectrum/test_spectrum_plot[ms_bokeh-kwargs0].html (1)
21-33: Snapshot IDs regenerated as expected.The element IDs and document references are dynamically generated during snapshot creation and differ on each regeneration. This is expected behavior for Bokeh HTML snapshots.
test/__snapshots__/test_peakmap/test_peakmap_plot[ms_plotly-kwargs2].json (2)
7-10: Binary-encoded data format is consistent with Plotly's typed array serialization.The
x,y, andcolorarrays are now serialized using Plotly's binary-encoded format (dtype+bdata). This is the expected output when usingplotly.io.to_json()with certain data types.
1728-1739: Newscattermaptrace type added to layout template.This addition likely reflects a Plotly library update that includes
scattermapas a new trace type in the default template. This is expected behavior when regenerating snapshots with an updated Plotly version.pyopenms_viz/testing/PlotlySnapshotExtension.py (4)
2-2: LGTM!The
WriteModeimport is correctly added to support the binary serialization mode required by syrupy 5.0.0+.
14-14: LGTM!Setting
_write_mode = WriteMode.BINARYaligns with the pattern used inBokehSnapshotExtensionandMatplotlibSnapshotExtensionas shown in the relevant code snippets.
17-30: LGTM!The
matches()method correctly handles both bytes and string inputs by decoding bytes to UTF-8 strings before JSON parsing. This defensive approach ensures compatibility with the binary write mode.
72-82: LGTM!The
serialize()method correctly returns UTF-8 encoded bytes, consistent with the binary write mode and matching the pattern inBokehSnapshotExtension.serialize().Regarding the static analysis hint about unused
kwargs: this parameter is part of the parent class interface and should be retained for compatibility even if unused.test/__snapshots__/test_spectrum/test_spectrum_plot[ms_plotly-kwargs3].json (4)
8-15: Binary-encoded data format applied consistently.The
xandytrace data uses Plotly's typed array serialization format, consistent with the other snapshot updates in this PR.
17-152: Customdata expansion supports annotation alignment.The
customdataarray now contains 3 entries per data point (matching the expanded x/y data for line rendering: start point, peak, end point). This aligns with the PR objective of fixing annotation rendering for single-trace spectra by ensuring hover data matches the expanded trace coordinates.
1050-1111: Annotations are correctly present in the snapshot.The annotations array contains the expected peak labels (100.5332, 74.1324, 200.4232, 160.2, 101.545), confirming that the annotation bug fix is working as intended for this single-trace spectrum plot.
551-562: Scattermap trace type added consistently.The
scattermaplayout template entry matches the additions in other Plotly snapshots, reflecting the updated Plotly library version.test/__snapshots__/test_spectrum/test_spectrum_plot_with_peak_color[ms_plotly-kwargs1].json (3)
9-16: Binary-encoded data format for "black" trace.The
xandyarrays for the first trace use Plotly's typed array serialization, consistent with the PR-wide snapshot format updates.
196-203: Binary-encoded data format for "red" trace.The second trace's
xandyarrays also use the typed array format, maintaining consistency across all traces in the snapshot.
616-627: Scattermap trace type added consistently.The
scattermaplayout template entry is consistent with the other Plotly snapshot updates in this PR.test/__snapshots__/test_chromatogram/test_chromatogram_plot[ms_plotly-kwargs1].json (2)
2-3459: Snapshot data serialization looks consistent.Binary-encoded x/y payloads and refreshed customdata appear coherent for the updated serializer output.
3855-3866: Template update looks fine.The added scattermap template entry aligns with the new snapshot structure.
test/__snapshots__/test_chromatogram/test_chromatogram_plot[ms_plotly-kwargs2].json (2)
2-3459: Snapshot data serialization looks consistent.Binary-encoded x/y payloads and refreshed customdata appear coherent for the updated serializer output.
3855-3866: Template update looks fine.The added scattermap template entry aligns with the new snapshot structure.
test/__snapshots__/test_peakmap/test_peakmap_plot[ms_plotly-kwargs0].json (2)
4-74: Snapshot data serialization looks consistent.Binary-encoded payloads and shape metadata appear coherent for the updated serializer output.
471-482: Template update looks fine.The added scattermap template entry aligns with the new snapshot structure.
test/__snapshots__/test_spectrum/test_spectrum_plot[ms_plotly-kwargs0].json (2)
8-153: Snapshot data serialization looks consistent.Binary-encoded x/y and refreshed customdata align with the updated serializer output.
551-562: Template update looks fine.The added scattermap template entry aligns with the new snapshot structure.
test/__snapshots__/test_peakmap/test_peakmap_plot[ms_plotly-kwargs3].json (1)
7-10: Snapshot updates align with the new encoded payload format.No issues noted with the regenerated snapshot data and layout template updates.
Also applies to: 60-67, 221-224, 274-281, 435-438, 488-495, 649-652, 702-709, 863-866, 916-923, 1077-1080, 1130-1137, 1291-1294, 1344-1351, 1896-1907
test/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_plotly-kwargs0].json (1)
9-16: Snapshot updates look consistent with encoded trace arrays.No concerns with the regenerated snapshot data.
Also applies to: 46-53, 83-90, 156-163, 193-200, 230-237, 674-685
test/__snapshots__/test_peakmap/test_peakmap_mz_im[ms_plotly].json (1)
7-10: Snapshot updates for encoded trace/customdata look consistent.No issues detected in the regenerated snapshot.
Also applies to: 60-67, 69-73, 471-482
pyopenms_viz/testing/BokehSnapshotExtension.py (1)
9-10: Binary snapshot handling is consistent with bytes serialization.Please verify this against the minimum supported syrupy version in CI.
Also applies to: 43-44, 78-81, 144-152
pyopenms_viz/testing/MatplotlibSnapshotExtension.py (1)
2-4: Binary write mode setup looks aligned with PNG snapshots.Please verify syrupy WriteMode.BINARY support and PIL availability in the test environment.
Also applies to: 13-17
test/__snapshots__/test_spectrum/test_spectrum_binning[ms_plotly-kwargs3].json (2)
8-15: Binary-encoded data snapshot looks consistent.
No concerns with the updated snapshot encoding.
461-472: Template addition looks fine.
The new scattermap template entry is consistent with regenerated snapshots.test/__snapshots__/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs1].html (1)
15-34: Snapshot embed wiring is internally consistent.
IDs and docs_json mapping updates look coherent for the regenerated snapshot.test/__snapshots__/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs3].html (1)
15-34: Snapshot embed wiring is internally consistent.
The updated IDs and render_items mapping look correct for the regenerated snapshot.test/__snapshots__/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs2].html (1)
15-34: Snapshot embed wiring is internally consistent.
IDs and embedded mapping updates look correct for the snapshot.test/__snapshots__/test_spectrum/test_spectrum_binning[ms_bokeh-kwargs0].html (1)
15-34: Snapshot embed wiring is internally consistent.
The regenerated IDs and render_items mapping look coherent here.test/__snapshots__/test_peakmap_marginals/test_peakmap_marginals[ms_plotly].json (1)
1-1772: Snapshot updates align with infrastructure changes.The changes in this snapshot file reflect:
- Migration to binary-encoded data format (
dtype/bdata) for coordinates, colors, and customdata - consistent with syrupy 5.0.0 serialization updates- Addition of
scattermaptrace type in the layout template, mirroring the existingscattermapboxconfigurationThe JSON structure is valid, and data shapes (
"168, 4") are consistent across related fields.test/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_plotly-kwargs2].json (1)
1-1252: Multi-trace annotated spectrum snapshot validates the fix.This snapshot correctly demonstrates:
- Each trace's
customdataarray length matches itsxcoordinate length (e.g., 3 entries for 3-point stem segments, 9 entries for multi-peak traces)- Annotations (lines 1173-1234) are properly configured with correct peak positions and ion labels
- Binary encoding (
dtype/bdata) is consistently applied across all 6 tracesThe data alignment ensures hover tooltips display correct information at each point of the spectrum visualization.
test/__snapshots__/test_spectrum/test_spectrum_with_annotations[ms_plotly-kwargs1].json (2)
8-154: Core fix validated:customdatanow aligns with trace length for single-trace spectrum.This snapshot demonstrates the fix for issue
#88. The single-trace spectrum data now has:
- 27
x/ypoints (9 peaks × 3 points each for stem-plot segments: baseline→peak→baseline)- 27
customdataentries (lines 17-152), properly expanded to matchPreviously,
customdatalikely had only 9 entries (one per peak) whilexhad 27 points, causing the annotation/hover mismatch. The triplication of customdata entries ensures each point in the trace has corresponding hover information.
551-562: Template addition is consistent.The
scattermapentry mirrors the existingscattermapboxconfiguration and is consistent across all snapshot files in this PR.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| f.write(data) | ||
|
|
||
| def serialize(self, data: SerializableData, **kwargs: Any) -> str: | ||
| def serialize(self, data: SerializableData, **kwargs: Any) -> bytes: |
There was a problem hiding this comment.
Silence unused kwargs to avoid Ruff ARG002.
If the signature must remain for the base class, mark it ignored.
💡 Suggested fix
- def serialize(self, data: SerializableData, **kwargs: Any) -> bytes:
+ def serialize(self, data: SerializableData, **kwargs: Any) -> bytes: # noqa: ARG002📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def serialize(self, data: SerializableData, **kwargs: Any) -> bytes: | |
| def serialize(self, data: SerializableData, **kwargs: Any) -> bytes: # noqa: ARG002 |
🧰 Tools
🪛 Ruff (0.14.11)
142-142: Unused method argument: kwargs
(ARG002)
🤖 Prompt for AI Agents
In `@pyopenms_viz/testing/BokehSnapshotExtension.py` at line 142, The serialize
method is currently leaving **kwargs unused which triggers Ruff ARG002; update
the method signature in BokehSnapshotExtension.py (serialize(self, data:
SerializableData, **kwargs: Any) -> bytes) to silence the warning by marking the
var as intentionally unused—e.g., change **kwargs to **_kwargs or explicitly
reference kwargs in the body with a no-op assignment like _ = kwargs or add a
noqa ARG002 comment—so the signature stays compatible but Ruff no longer flags
it.
| pytest | ||
| syrupy | ||
| syrupy>=5.0.0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for syrupy usage in the codebase
rg "syrupy" --type py -A 3 -B 1Repository: OpenMS/pyopenms_viz
Length of output: 5457
🏁 Script executed:
# Check if there are any other requirements files mentioning syrupy
fd -e txt -e in . | xargs grep -l "syrupy" 2>/dev/null || echo "No other matches found"Repository: OpenMS/pyopenms_viz
Length of output: 122
🏁 Script executed:
# Check git history to see what version was previously used
git log --oneline -p -- requirements-dev.txt | head -100Repository: OpenMS/pyopenms_viz
Length of output: 322
🌐 Web query:
syrupy 5.0.0 release notes breaking changes
💡 Result:
Breaking changes in syrupy 5.0.0 (per the release/changelog):
- License switched to MIT (replacing the previous Apache-2.0). [1][2]
- Minimum Python requirement raised to Python 3.10 (older 3.8 support dropped). [2]
- Minimum required pytest version raised to pytest 8. [2]
Sources: PyPI release listing for 5.0.0. [1]; RPM/changelog that lists the 5.0.0 breaking changes. [2]
Ensure pytest version constraint is compatible with syrupy 5.0.0.
Syrupy 5.0.0 requires pytest 8 or later; add pytest>=8 to line 1 to guarantee compatibility with the syrupy dependency on line 2. The custom snapshot extensions correctly use syrupy's stable public extension APIs.
🤖 Prompt for AI Agents
In `@requirements-dev.txt` around lines 1 - 2, Update the pytest requirement to
ensure compatibility with syrupy>=5.0.0 by changing the pytest entry in
requirements-dev.txt from "pytest" to "pytest>=8"; this guarantees pytest
version >=8 for the "pytest" dependency to satisfy "syrupy>=5.0.0".
| } | ||
| </style> | ||
| <script type="text/javascript" src="https://cdn.bokeh.org/bokeh/release/bokeh-3.6.0.min.js"></script> | ||
| <script type="text/javascript" src="https://cdn.bokeh.org/bokeh/release/bokeh-3.4.1.min.js"></script> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Bokeh version in project dependencies
rg -n "bokeh" pyproject.toml requirements*.txt setup.py setup.cfg 2>/dev/null || echo "No matches found in standard dependency files"
# Also search broadly for bokeh version specs
find . -type f \( -name "pyproject.toml" -o -name "requirements*.txt" -o -name "setup.py" -o -name "setup.cfg" \) | head -20Repository: OpenMS/pyopenms_viz
Length of output: 762
Regenerate snapshot to match pinned Bokeh dependency version.
The snapshot file loads bokeh-3.4.1, but requirements.txt pins bokeh==3.6.0. The 3.4.1 version in the snapshot appears to be outdated and should be regenerated to align with the project's pinned dependency.
🤖 Prompt for AI Agents
In `@test/__snapshots__/test_spectrum/test_spectrum_plot`[ms_bokeh-kwargs0].html
at line 15, The snapshot HTML contains a hardcoded script tag referencing
bokeh-3.4.1; update the test snapshot for
test_spectrum_plot[ms_bokeh-kwargs0].html to reference the project's pinned
bokeh version (bokeh-3.6.0) by regenerating the snapshot (re-run the snapshot
generation/test that produces this file) so the embedded script src becomes
https://cdn.bokeh.org/bokeh/release/bokeh-3.6.0.min.js and the test artifacts
match requirements.txt.
| } | ||
| </style> | ||
| <script type="text/javascript" src="https://cdn.bokeh.org/bokeh/release/bokeh-3.6.0.min.js"></script> | ||
| <script type="text/javascript" src="https://cdn.bokeh.org/bokeh/release/bokeh-3.4.1.min.js"></script> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Bokeh version in project dependencies
echo "=== Checking project dependencies ==="
find . -maxdepth 2 \( -name "pyproject.toml" -o -name "requirements*.txt" -o -name "setup.py" \) -type f | while read f; do
if grep -i "bokeh" "$f" 2>/dev/null; then
echo "--- Found in $f ---"
grep -i "bokeh" "$f"
fi
done
echo -e "\n=== Checking snapshot bokeh versions ==="
rg "bokeh-[0-9]+\.[0-9]+\.[0-9]+" test/__snapshots__/ --max-count=10 || echo "No matches found"Repository: OpenMS/pyopenms_viz
Length of output: 7505
Snapshots use Bokeh 3.4.1 but locked dependencies specify 3.6.0/3.6.2—regenerate with the correct version.
All snapshots consistently reference bokeh-3.4.1.min.js, while the project's pinned dependencies are bokeh==3.6.0 (in requirements.txt and .binder/requirements.txt) and bokeh==3.6.2 (in docs/requirements.txt). The 3.4.1 version is only the minimum supported version in pyproject.toml (bokeh>=3.4.1). This mismatch indicates snapshots were regenerated in an environment with bokeh 3.4.1 installed, creating a disconnect between the tested version and the deployed version. Regenerate snapshots with the correct bokeh version from the locked dependencies.
🤖 Prompt for AI Agents
In `@test/__snapshots__/test_spectrum/test_spectrum_plot`[ms_bokeh-kwargs2].html
at line 15, The snapshot HTML references "bokeh-3.4.1.min.js" but locked deps
require bokeh 3.6.x; re-generate the failing snapshot file (the snapshot named
test_spectrum_plot[ms_bokeh-kwargs2].html that contains the script tag
"bokeh-3.4.1.min.js") in an environment with the pinned bokeh version (install
bokeh==3.6.0 or 3.6.2 per requirements.txt / docs/requirements.txt), run the
test that creates the Bokeh output to update the snapshot (so the script tag
becomes the correct bokeh-3.6.x URL), and commit the regenerated snapshot.
Ensure your test environment uses the same dependency resolution as CI to avoid
future mismatches.
|
Closing in favour of #140 |
Pull request was closed
This fixes issue 88 where the annotations do not appear correctly when plotting a single trace spectrum.
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.