feat: add .ms_plot accessor to Pandas DataFrames for API parity#165
feat: add .ms_plot accessor to Pandas DataFrames for API parity#165Sanjith1013 wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a Pandas DataFrame accessor to expose the project’s plotting API under df.ms_plot(...), aiming to align the call site with the existing “namespace” style used elsewhere and keep delegation routed through Pandas’ plotting backend mechanism.
Changes:
- Registered a new Pandas DataFrame accessor (
ms_plot) that delegates toDataFrame.plot(...). - Imported the accessor module from
pyopenms_viz.__init__to enable registration via import side effects. - Added a syrupy snapshot test (and snapshot artifact) for the new accessor.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
pyopenms_viz/_pandas_accessor.py |
Introduces the ms_plot DataFrame accessor that forwards calls to DataFrame.plot. |
pyopenms_viz/__init__.py |
Imports the accessor module for registration side effects when pyopenms_viz is imported. |
test/test_pandas_accessor.py |
Adds a snapshot test covering df.ms_plot(kind="spectrum", ...). |
test/__snapshots__/test_pandas_accessor/test_pandas_ms_plot_snapshot.json |
Stores expected Plotly JSON output for the new accessor test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import types | ||
| from pathlib import Path | ||
| from ._core import plot_chromatogram, plot_spectrum, plot_mobilogram, plot_peakmap | ||
| from . import _pandas_accessor # Import for side effects: registers Pandas 'ms_plot' namespace. noqa: F401 |
| backend="ms_plotly", | ||
| show_plot=False | ||
| ) | ||
|
|
| @pytest.fixture(autouse=True) | ||
| def load_backend(): | ||
| """ | ||
| Safely override the global backend so the syrupy snapshot fixture | ||
| in conftest.py knows which extension to load, then reset it. | ||
| """ | ||
| original_backend = pd.options.plotting.backend | ||
| pd.options.plotting.backend = "ms_plotly" | ||
|
|
||
| yield | ||
|
|
||
| # Teardown: restore the original state | ||
| pd.options.plotting.backend = original_backend |
| @pytest.fixture(autouse=True) | ||
| def load_backend(): | ||
| """ | ||
| Safely override the global backend so the syrupy snapshot fixture | ||
| in conftest.py knows which extension to load, then reset it. | ||
| """ |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRegisters a new Pandas DataFrame accessor Changes
Sequence DiagramsequenceDiagram
participant User as User Code
participant Accessor as ms_plot Accessor
participant DataFrame as pandas.DataFrame
participant Backend as Plot Backend
participant Figure as Figure Renderer
User->>Accessor: df.ms_plot(*args, **kwargs)
Accessor->>Accessor: __call__ receives args/kwargs
Accessor->>DataFrame: DataFrame.plot(*args, **kwargs)
DataFrame->>Backend: route to configured plotting backend
Backend->>Figure: construct figure (traces, layout, annotations)
Figure-->>Backend: rendered figure object
Backend-->>DataFrame: return figure
DataFrame-->>Accessor: return figure
Accessor-->>User: return figure object
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pyopenms_viz/__init__.py`:
- Line 13: The inline "noqa: F401" text is not in canonical comment form and may
be ignored; update the import line that references _pandas_accessor so the
suppression is an actual comment token at the end of the line (e.g. ensure "#
noqa: F401" appears as a proper comment after the explanatory comment) so
linters reliably detect the F401 ignore for the from . import _pandas_accessor
statement.
In `@test/test_pandas_accessor.py`:
- Around line 41-42: The snapshot assertion is unstable because Plotly's
version-dependent layout.template is included; modify the serialization step
before comparing snapshot to remove or normalize figure.layout.template by
calling the Figure.to_dict() representation (use to_dict() instead of
to_plotly_json()) and delete or set figure_dict["layout"].pop("template", None)
(or equivalent) on the exported dict (the variables involved are out and
snapshot and the nested layout.template field) so that snapshot == out compares
a template-free representation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9eadb01-08c8-42b3-96a5-ddd49ebf0896
📒 Files selected for processing (4)
pyopenms_viz/__init__.pypyopenms_viz/_pandas_accessor.pytest/__snapshots__/test_pandas_accessor/test_pandas_ms_plot_snapshot.jsontest/test_pandas_accessor.py
There was a problem hiding this comment.
Pull request overview
Adds a Pandas DataFrame.ms_plot accessor to mirror the “namespace accessor” style API and delegates plotting to DataFrame.plot() so existing Pandas plotting backend configuration continues to work unchanged.
Changes:
- Added
pyopenms_viz._pandas_accessor.PandasPlotAccessorand registered it via@pd.api.extensions.register_dataframe_accessor("ms_plot"). - Ensured the accessor is registered on
import pyopenms_vizvia an import-for-side-effects inpyopenms_viz/__init__.py. - Added snapshot-based tests (Plotly JSON, Matplotlib PNG, Bokeh HTML) for
df.ms_plot(...).
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
pyopenms_viz/_pandas_accessor.py |
Registers DataFrame.ms_plot accessor and delegates calls to DataFrame.plot(). |
pyopenms_viz/__init__.py |
Imports the accessor module for registration side-effects. |
test/test_pandas_accessor.py |
Adds a snapshot test exercising the new accessor across configured backends. |
test/__snapshots__/test_pandas_accessor/test_pandas_ms_plot_snapshot[ms_plotly].json |
Plotly backend expected snapshot for accessor test. |
test/__snapshots__/test_pandas_accessor/test_pandas_ms_plot_snapshot[ms_matplotlib].png |
Matplotlib backend expected snapshot for accessor test. |
test/__snapshots__/test_pandas_accessor/test_pandas_ms_plot_snapshot[ms_bokeh].html |
Bokeh backend expected snapshot for accessor test. |
test/__snapshots__/test_pandas_accessor/test_pandas_ms_plot_snapshot.json |
Additional JSON snapshot added (appears redundant vs the parameterized Plotly snapshot). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| "data": [ | ||
| { | ||
| "line": { | ||
| "color": "#4575B4" |
|
Hi @singjc, Just a quick update: I've resolved all the CodeRabbit feedback in the latest commits (fixed the noqa linting format and stripped the volatile Plotly layout.template data to make the new tests resilient across Plotly versions). However, the CI is still throwing a red check due to two unrelated snapshot test failures in test/test_peakmap_marginals.py (test_peakmap_marginals[ms_bokeh] and test_peakmap_mz_im[ms_bokeh]). It looks like the ms_bokeh backend is generating dynamic HTML UUIDs, which is causing a strict string mismatch against the stored syrupy snapshots. My added tests for the Pandas accessor are passing perfectly. Let me know if you'd like me to run a global --snapshot-update to sync the Bokeh UUIDs for the repo, or if you prefer to handle that flakiness separately. |
|
Hi @singjc, |
Description
Following up on the discussion in PR #164 regarding API unification, this PR introduces the
.ms_plotnamespace directly to Pandas DataFrames.By using
@pd.api.extensions.register_dataframe_accessor, users can now use the exact same syntax (df.ms_plot()) regardless of whether they are working with Pandas or Polars.Technical Details:
_pandas_accessor.pyto securely delegate.ms_plot()calls to the nativepandas.DataFrame.plot()backend.pd.options.plotting.backend = "ms_plotly", are perfectly respected without duplicating any rendering logic.__init__.pyfor seamless import side-effects.Testing
test_pandas_accessor.pyusing thesyrupysnapshot framework to mathematically guarantee that the Plotly JSON output matches the expected structure.load_backendfixture to prevent global state leaks inconftest.py.cc: @singjc - Thanks for the great suggestion to unify the API!
Summary by CodeRabbit
New Features
Tests