Fix/plotly spec tooltip - #141
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. 📝 WalkthroughWalkthroughTooltip handling and alignment were changed: line-plot conversion now carries and aligns per-point hover data, BasePlot.generate gained a fixed_tooltip_for_trace parameter, backends forward the flag, and many tests/snapshots updated to reflect expanded/stacked Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as plot() / Caller
participant CFLP as convert_for_line_plots
participant Renderer as Renderer (Plotly / Bokeh / Matplotlib)
participant Tooltip as _add_tooltips / Tooltip Handler
Caller->>CFLP: pass data (+ optional custom_hover_data, reference data)
CFLP->>CFLP: convert points to line-format (x,y) and repeat/stack custom_hover_data
CFLP-->>Caller: (converted_data, aligned_custom_hover_data)
Caller->>Renderer: render(converted_data, aligned_custom_hover_data, fixed_tooltip_for_trace)
Renderer->>Tooltip: _add_tooltips(tooltips, custom_hover_data, fixed_tooltip_for_trace)
Tooltip->>Tooltip: apply fixed-per-trace or per-point slicing based on flag
Tooltip-->>Renderer: traces with attached hover data
Renderer-->>Caller: final interactive plot
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 issue #88 by ensuring that tooltip hover data correctly aligns with the transformed spectrum data in Plotly plots. The fix updates the custom_hover_data array to match the 3-point peak representation used in line plots.
Changes:
- Modified
convert_for_line_plots()to transform custom hover data alongside spectrum data, repeating each data point 3 times to match the line plot format (0, peak, 0) - Updated the spectrum plotting flow to properly pass and handle custom hover data through the conversion process
- Added support for mirror spectrum tooltips by stacking hover data arrays
- Removed debug print statement and changed
fixed_tooltip_for_tracedefault toFalse
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pyopenms_viz/_core.py | Core fix that modifies convert_for_line_plots() to return both transformed data and custom hover data, handles grouped data correctly, and adds mirror spectrum hover data support |
| pyopenms_viz/_plotly/core.py | Changed default parameter fixed_tooltip_for_trace from True to False to enable per-point tooltips |
| test/snapshots/test_spectrum/*.json | Updated test snapshots reflecting the corrected customdata arrays with 3x repetition for each peak |
💡 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyopenms_viz/_plotly/core.py (1)
141-162: Tooltip slicing misaligns when hover-data originates from reordered data.
get_spectrum_tooltip_datareorders data viaconcat(..., ignore_index=True)before creatingcustom_hover_data, butconvert_for_line_plotstries to index that array using indices from the original ungrouped data. Sincecustom_hover_datais a numpy array indexed by position (not labels), indexing with original dataframe indicescustom_hover_data[df.index]selects wrong rows when groups are interleaved or non-sequential.For grouped plots (peakmap/scatter with
byparameter), this causes tooltips to misalign with their corresponding traces.Fix: Either reorder
custom_hover_datato match original data order beforeconvert_for_line_plots, or pass per-group hover arrays instead of using index-based slicing.
🤖 Fix all issues with AI agents
In `@pyopenms_viz/_core.py`:
- Around line 1090-1115: The convert_for_line_plots method fails when DataFrame
indices are non-default; fix by resetting the DataFrame index to positional
integers at the start (e.g., data = data.reset_index(drop=True)) so subsequent
grouping and positional slicing of custom_hover_data works; ensure you still
call self.to_line with the reset data, and when extracting per-group hover data
use the positional indices (group_hover_data = custom_hover_data[df.index])
after reset (or reset each group with df = df.reset_index(drop=True) inside the
loop) so repeat(...)/vstack(...) produce correctly ordered tooltips.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyopenms_viz/_matplotlib/core.py (1)
231-241: Fix the breaking signature change inMATPLOTLIBPlot.generate.Line 231 now requires
_fixed_tooltip_for_trace, but many call sites still pass only two args, triggering the CI TypeError. Add a default value (or update all call sites) to restore compatibility.🔧 Proposed fix
- def generate(self, tooltips, custom_hover_data, _fixed_tooltip_for_trace) -> Axes: + def generate( + self, tooltips, custom_hover_data, _fixed_tooltip_for_trace=True + ) -> Axes:
🤖 Fix all issues with AI agents
In `@pyopenms_viz/_core.py`:
- Around line 816-831: The code unconditionally calls
np.vstack([custom_hover_data, reference_custom_hover_data]) which will fail when
custom_hover_data is None (non-interactive backends); update the block around
convert_for_line_plots/get_line_renderer so you only stack when tooltips (or
custom_hover_data) exists — e.g., if tooltips and custom_hover_data is not None
then do the vstack, otherwise set custom_hover_data =
reference_custom_hover_data (or leave as None) before calling
mirrorSpectrumPlot.generate; ensure the change touches the variables
custom_hover_data, reference_custom_hover_data and the
mirrorSpectrumPlot.generate(...) call so non-interactive backends do not trigger
np.vstack on None.
🧹 Nitpick comments (1)
pyopenms_viz/_core.py (1)
1090-1115: Update the return type annotation to match the new tuple return.
convert_for_line_plotsnow returns(DataFrame, custom_hover_data)but the annotation still says-> DataFrame. This makes the public API misleading for type checkers and callers.🔧 Proposed fix
- def convert_for_line_plots( - self, data: DataFrame, x: str, y: str, custom_hover_data=None - ) -> DataFrame: + def convert_for_line_plots( + self, + data: DataFrame, + x: str, + y: str, + custom_hover_data=None, + ) -> tuple[DataFrame, np.ndarray | None]:
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pyopenms_viz/_matplotlib/core.py`:
- Around line 231-233: The generate method's keyword parameter name doesn't
match the BasePlot contract causing TypeError; rename the parameter in def
generate(self, tooltips, custom_hover_data, _fixed_tooltip_for_trace=True) to
fixed_tooltip_for_trace to match BasePlot, and inside the method explicitly
discard the unused argument (e.g., assign to _ or use del) to silence ARG002
while preserving behavior; update any internal references to
_fixed_tooltip_for_trace to the new name (generate, BasePlot compatibility).
…eference spectrum
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
pyopenms_viz/_core.py:422
- Overridden method signature does not match call, where it is passed too many arguments. Overriding method method BOKEHPlot._add_tooltips matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method PLOTLYPlot._add_tooltips matches the call.
def _add_tooltips(self, tooltips, custom_hover_data):
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@test/__snapshots__/test_spectrum/test_mirror_spectrum`[ms_bokeh].html:
- Line 15: Update the snapshot to use the current Bokeh release and fix the
HoverTool tooltip mismatch: replace the bokeh script reference from 3.4.1 to
3.6.0 (so the snapshot matches the pinned environment) and either add a
native_id field to the ColumnDataSource used to render the plot or change the
HoverTool tooltip configuration to reference an existing field (e.g., index, mz,
or intensity) instead of `@native_id`; locate the tooltip configuration and the
ColumnDataSource declaration in the test_mirror_spectrum[ms_bokeh] snapshot to
make these consistent.
- Around line 23-24: convert_for_line_plots() is dropping the native_id field so
the ColumnDataSource created for the line plot only contains index, mz, and
intensity causing tooltips referencing `@native_id` to be empty; update
convert_for_line_plots() to detect and preserve any auxiliary fields
(specifically native_id) when transforming spectrum data into the line-plot
arrays (ensure native_id is expanded/aligned to the per-point arrays like
mz/intensity and included in the returned data map used to build the
ColumnDataSource), and verify the ColumnDataSource creation logic uses that
preserved native_id so the HoverTool tooltip `@native_id` shows the correct
values.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pyopenms_viz/_core.py`:
- Around line 1090-1098: The method convert_for_line_plots currently returns a
tuple (DataFrame, custom_hover_data) but is annotated as returning DataFrame;
update the signature to reflect the actual return type (e.g., Tuple[DataFrame,
Optional[...]]) and add the necessary typing imports (from typing import Tuple,
Optional) or appropriate generic for custom_hover_data; ensure the return
annotation matches the actual returned types from convert_for_line_plots and
keep the existing behavior using to_line and custom_hover_data.
♻️ Duplicate comments (2)
pyopenms_viz/_core.py (2)
826-831: Guardnp.vstackwhencustom_hover_dataisNone.This issue was previously flagged: if
_create_tooltipsreturnsNone(e.g., non-interactive backends like Matplotlib),np.vstackwill raise an error.🔧 Proposed fix
- # Stack reference custom hover data to custom hover data - custom_hover_data = np.vstack( - [custom_hover_data, reference_custom_hover_data] - ) + # Stack reference custom hover data to custom hover data + if custom_hover_data is not None and reference_custom_hover_data is not None: + custom_hover_data = np.vstack( + [custom_hover_data, reference_custom_hover_data] + ) + else: + custom_hover_data = None
1103-1113: Index-based slicing may fail with non-default DataFrame indices.This concern was previously flagged: when
datahas non-contiguous or non-zero-based indices (e.g., from prior filtering),custom_hover_data[df.index]performs positional indexing that may not align correctly, causing tooltip misalignment orIndexError.🔧 Proposed fix - reset index at method start
def convert_for_line_plots( self, data: DataFrame, x: str, y: str, custom_hover_data=None ) -> Tuple[DataFrame, np.ndarray | None]: + # Reset index to ensure positional alignment with custom_hover_data + data = data.reset_index(drop=True) + if self.by is None:
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
pyopenms_viz/_core.py:422
- Overridden method signature does not match call, where it is passed too many arguments. Overriding method method BOKEHPlot._add_tooltips matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method PLOTLYPlot._add_tooltips matches the call.
def _add_tooltips(self, tooltips, custom_hover_data):
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
pyopenms_viz/_core.py:422
- Overridden method signature does not match call, where it is passed too many arguments. Overriding method method BOKEHPlot._add_tooltips matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method PLOTLYPlot._add_tooltips matches the call.
def _add_tooltips(self, tooltips, custom_hover_data):
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Convert to line plot format, and update custom hover data accordingly. We modify the spectrum df to plot line plots, such that each peak is represented by 3 points (x, 0), (x, y), (x, 0). custom hover data is repeated accordingly to match (also matches order when grouped by "by" column) | ||
| spectrum, custom_hover_data = self.convert_for_line_plots( | ||
| spectrum, self.x, self.y, custom_hover_data | ||
| ) |
There was a problem hiding this comment.
There's a potential issue when peak binning is enabled. The custom_hover_data array is created from self.data (line 771-772), but spectrum is the prepared/binned version which may have different rows and indices. When convert_for_line_plots tries to access custom_hover_data[df.index] (line 1107), the indices may not match if binning has aggregated peaks. Consider creating custom_hover_data from the prepared spectrum DataFrame instead of self.data to ensure consistency.
There was a problem hiding this comment.
This comment seems valid. Should likely do this
jcharkow
left a comment
jcharkow
left a comment
There was a problem hiding this comment.
Of the three, I think that this solution is the best since it also addresses the reference spectrum, it seems to be less hacky than the first implementation, and is not confusing like my implementation.
I think that co-pilot made some good suggestions specifically with consistencies with fixed_tooltip_for_trace vs _fixed_tooltip_for_trace and some documentation suggestions.
After these suggestions are addressed I think it is ready to merge.
| # Convert to line plot format, and update custom hover data accordingly. We modify the spectrum df to plot line plots, such that each peak is represented by 3 points (x, 0), (x, y), (x, 0). custom hover data is repeated accordingly to match (also matches order when grouped by "by" column) | ||
| spectrum, custom_hover_data = self.convert_for_line_plots( | ||
| spectrum, self.x, self.y, custom_hover_data | ||
| ) |
There was a problem hiding this comment.
This comment seems valid. Should likely do this
| custom_hover_data = repeat(custom_hover_data, 3, axis=0) | ||
| return DataFrame({x: x_data, y: y_data}), custom_hover_data |
There was a problem hiding this comment.
I like the implementation that chat-GPT came up with previously of modifying the dataframe directly with .loc as I think it looks cleaner. I've modified it here for hover data, perhaps this is a good solution because it preserves all columns? I had some issues before where native_id=??? with bokeh and this should addresses that.
What are your thoughts?
# Repeat all columns
custom_hover_data = custom_hover_data.loc[custom_hover_data.index.repeat(3)].reset_index(drop=True)
# Zero out baseline for y (same logic as before)
custom_hover_data.loc[custom_hover_data.index % 3 != 1, y] = 0
There was a problem hiding this comment.
I'm not sure this applies to custom_hover_data, which is a numpy array, not a pandas dataframe. The issue with column preservation is probably when the data is being manipulated, with dropped columns maybe? I think we would have to look at that in another PR, I think I saw some TODOs related to this in the codebase.
There was a problem hiding this comment.
I always thought that custom_hover_data was a pandas dataframe since you had to specify the columns as well?
We can look more into this in another PR though
There was a problem hiding this comment.
We use the columns to extract the underlying arrays in the pandas dataframe (_plotly/core.py#L559 and _plotly/core.py#L574).
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
pyopenms_viz/_core.py:422
- Overridden method signature does not match call, where it is passed too many arguments. Overriding method method PLOTLYPlot._add_tooltips matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method BOKEHPlot._add_tooltips matches the call.
def _add_tooltips(self, tooltips, custom_hover_data):
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…tlib, and Plotly backends
…r data handling in SpectrumPlot
…nsure index consistency
…viz into fix/plotly_spec_tooltip
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
pyopenms_viz/_core.py:433
- Overridden method signature does not match call, where it is passed too many arguments. Overriding method method BOKEHPlot._add_tooltips matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method PLOTLYPlot._add_tooltips matches the call.
def _add_tooltips(self, tooltips, custom_hover_data):
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@jcharkow, is this good to merge? |
|
Seems that deploy documentation is still failing but thats another issue so I'll merge it. |
Yet another solution for fixing #88. This approach updates the
custom_hover_dataarray to be of the same length as the 3-point peak transformed spectrum data. This also works when grouping the spectrum df by meta data (i.e.spectrum_df.plot(kind='spectrum', x='mz', y='intensity', by="ms_level"), as well as works when supplying a mirror spectrum (i.e.spectrum_df.plot(kind='spectrum', x='mz', y='intensity', by="ms_level", reference_spectrum=predicted_df, mirror_spectrum=True).Summary by CodeRabbit
New Features
Improvements
Tests
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.