feat: add Polars support with version-locked dependencies#164
feat: add Polars support with version-locked dependencies#164Sanjith1013 wants to merge 16 commits into
Conversation
|
Hi @singjc, I’ve thoroughly reviewed the feedback and the closing notes from the previous attempt in PR #71. Based on your suggestions there, I have implemented the following specific changes: Avoided forced dependencies: Polars is now an optional dependency handled via dynamic imports. Safeguarded downstream logic: Instead of refactoring the core BasePlot manipulations, I implemented an upfront conversion to Pandas within the accessor. This ensures the rest of the library’s hardcoded Pandas methods (.empty, .copy(), etc.) remain untouched and fully functional. Namespace Collision Fix: Since Polars has reserved the .plot namespace, I used .ms_plot to keep the API clean and predictable. The code is now passing CI on all environments. Looking forward for any corrections |
There was a problem hiding this comment.
Pull request overview
Adds first-class plotting access from Polars DataFrames by registering a dedicated .ms_plot namespace that delegates to the existing pandas-based plotting backend, with dependency pins updated to include Polars + PyArrow.
Changes:
- Added
pyopenms_viz._polars_accessorto registerpolars.DataFrame.ms_plot. - Hooked accessor registration into package import via
pyopenms_viz.__init__. - Pinned
polarsandpyarrowinrequirements.txt.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| requirements.txt | Adds pinned Polars/PyArrow packages to the compiled dependency lockfile. |
| pyopenms_viz/_polars_accessor.py | Implements Polars DataFrame namespace accessor that converts to pandas and calls .plot(). |
| pyopenms_viz/init.py | Imports _polars_accessor for registration side effects at import time. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import warnings | ||
|
|
There was a problem hiding this comment.
warnings is imported but never used. Remove the import, or use it to emit an explicit warning when converting a Polars DataFrame to pandas (since this can be expensive and/or require extra deps).
| self._df = df | ||
|
|
||
| def __call__(self, *args, **kwargs): | ||
| pandas_df = self._df.to_pandas() |
There was a problem hiding this comment.
self._df.to_pandas() can fail at runtime if pyarrow (and sometimes even pandas) is not installed, which is likely for users relying on Polars being an optional dependency (pyproject.toml currently doesn't declare polars/pyarrow). Consider catching the ImportError/ModuleNotFoundError here and raising a clearer message instructing which extra(s) to install, or declare/organize a dedicated optional-dependency extra for polars+pyarrow so this path is always satisfied.
| pandas_df = self._df.to_pandas() | |
| try: | |
| pandas_df = self._df.to_pandas() | |
| except (ImportError, ModuleNotFoundError) as exc: | |
| raise RuntimeError( | |
| "Converting a Polars DataFrame to pandas failed because required " | |
| "optional dependencies are missing. Please install 'pandas' and " | |
| "'pyarrow' (and ensure 'polars' is installed), for example via:\n" | |
| " pip install pandas pyarrow\n" | |
| "or the appropriate optional-dependency extra provided by this package." | |
| ) from exc |
|
|
||
| def __call__(self, *args, **kwargs): | ||
| pandas_df = self._df.to_pandas() | ||
| return pandas_df.plot(*args, **kwargs) No newline at end of file |
There was a problem hiding this comment.
Polars support is introduced here, but the test suite currently only exercises pandas DataFrames. Add at least one test that constructs a small polars.DataFrame, calls .ms_plot(...), and asserts the same behavior/output as the equivalent pandas .plot(...) call (including a backend like ms_matplotlib to ensure the pandas plotting-backend integration still works through the delegation).
| return pandas_df.plot(*args, **kwargs) | |
| return pandas_df.ms_plot(*args, **kwargs) |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional Polars DataFrame support by importing a new module that registers a "ms_plot" Polars namespace. Introduces PolarsPlotAccessor which converts Polars DataFrames to pandas (with pyarrow check) and delegates plotting; inert when Polars is unavailable (POLARS_AVAILABLE flag). Changes
Sequence DiagramsequenceDiagram
participant User
participant PolarsDF as Polars DataFrame
participant PolarsAccessor as PolarsPlotAccessor
participant PandasDF as Pandas DataFrame
participant PlotAccessor as PlotAccessor (pyopenms_viz._core)
User->>PolarsDF: call `.ms_plot(*args, **kwargs)`
PolarsDF->>PolarsAccessor: accessor invoked
PolarsAccessor->>PolarsDF: df.to_pandas(use_pyarrow_extension_array=True)
PolarsDF->>PandasDF: conversion result
PolarsAccessor->>PlotAccessor: delegate to PlotAccessor(pandas_df)(*args, **kwargs)
PlotAccessor->>User: return plot object
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/_polars_accessor.py`:
- Line 24: Change the conversion to preserve Arrow-backed nullable types by
calling to_pandas with use_pyarrow_extension_array=True on the Polars DataFrame:
update the pandas_df assignment (pandas_df = self._df.to_pandas()) to pass
use_pyarrow_extension_array=True so nullable Polars types remain Arrow-backed
(look for the pandas_df variable and the use of self._df.to_pandas in
_polars_accessor).
- Around line 23-25: The __call__ in the Polars plot accessor currently converts
to pandas and calls pandas_df.plot(), which bypasses pyopenms_viz routing for
custom kinds; instead import and use PlotAccessor from pyopenms_viz (the
accessor defined in __init__.py) to delegate plotting so custom kinds like
"chromatogram", "mobilogram", "spectrum", and "peakmap" work for Polars
DataFrames. Update the Polars accessor's __call__ (the method that does
self._df.to_pandas()) to create/instantiate PlotAccessor with the converted
pandas df and forward *args/**kwargs to its __call__/plot entrypoint rather than
directly calling pandas_df.plot().
In `@requirements.txt`:
- Around line 310-311: Remove the manually appended lines "polars==1.38.1" and
"pyarrow==23.0.1" from the lockfile and instead add these packages to the input
manifest used by pip-compile (e.g., requirements.in or the appropriate top-level
input file), then re-run pip-compile to regenerate the lockfile so entries are
sorted and include provenance comments; verify the regenerated requirements.txt
contains the packages with proper comments and version pins rather than the
unsorted manual entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 522f98ba-aa02-4609-b036-f9541d9c2a9b
📒 Files selected for processing (3)
pyopenms_viz/__init__.pypyopenms_viz/_polars_accessor.pyrequirements.txt
| polars==1.38.1 | ||
| pyarrow==23.0.1 No newline at end of file |
There was a problem hiding this comment.
Regenerate the lockfile instead of appending packages manually.
Lines 310-311 break the pip-compile shape of this file: they are unsorted and missing the usual provenance comments. That makes the lockfile non-reproducible, and the next compile is likely to reshuffle or drop these entries unless the source manifest was updated too. Please add these deps to the input file and re-run pip-compile.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@requirements.txt` around lines 310 - 311, Remove the manually appended lines
"polars==1.38.1" and "pyarrow==23.0.1" from the lockfile and instead add these
packages to the input manifest used by pip-compile (e.g., requirements.in or the
appropriate top-level input file), then re-run pip-compile to regenerate the
lockfile so entries are sorted and include provenance comments; verify the
regenerated requirements.txt contains the packages with proper comments and
version pins rather than the unsorted manual entries.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # 2. Correct Routing: Use the library's internal PlotAccessor. | ||
| # This ensures custom backends like 'ms_plotly' are found automatically. | ||
| from pyopenms_viz._core import PlotAccessor | ||
| return PlotAccessor(pandas_df)(*args, **kwargs) No newline at end of file |
There was a problem hiding this comment.
Delegating to pyopenms_viz._core.PlotAccessor is likely broken here: _core.PlotAccessor defaults to backend='matplotlib' (ignoring pd.options.plotting.backend used throughout the docs/tests) and its backend loader imports pyopenms_viz.plotting._* modules, which don’t exist in this repo layout. Consider delegating to pandas_df.plot(*args, **kwargs) so pandas handles the ms_* backends via entry points and respects the global plotting backend option.
| # 2. Correct Routing: Use the library's internal PlotAccessor. | |
| # This ensures custom backends like 'ms_plotly' are found automatically. | |
| from pyopenms_viz._core import PlotAccessor | |
| return PlotAccessor(pandas_df)(*args, **kwargs) | |
| # 2. Correct Routing: Delegate to pandas' plotting API. | |
| # This lets pandas resolve ms_* backends via entry points and respect | |
| # the global `pd.options.plotting.backend` setting. | |
| return pandas_df.plot(*args, **kwargs) |
| if POLARS_AVAILABLE: | ||
| @pl.api.register_dataframe_namespace("ms_plot") | ||
| class PolarsPlotAccessor: | ||
| def __init__(self, df: pl.DataFrame): | ||
| self._df = df | ||
|
|
||
| def __call__(self, *args, **kwargs): | ||
| try: | ||
| # 1. Safety Catch: Use Arrow-backed arrays with a clear error fallback | ||
| # This prevents cryptic crashes if pyarrow is missing. | ||
| pandas_df = self._df.to_pandas(use_pyarrow_extension_array=True) | ||
| except (ImportError, ModuleNotFoundError) as exc: |
There was a problem hiding this comment.
New Polars plotting entrypoint is introduced here, but there are no tests covering that the ms_plot namespace is registered and that calling it produces the same output as the pandas path (including respecting pd.options.plotting.backend and/or backend='ms_*'). Adding a small pytest covering the conversion + delegation behavior would prevent regressions.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pyopenms_viz/_polars_accessor.py (2)
22-25: Align the recovery hint with the supported dependency set.The exception text tells users to run
pip install pyarrow, but this PR also introduces version-locked Polars/PyArrow dependencies. A bare install can pull a different Arrow version than the one this accessor was validated against, so the message should point to the documented install path or the compatible pinned version instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyopenms_viz/_polars_accessor.py` around lines 22 - 25, The RuntimeError raised in pyopenms_viz._polars_accessor (the message thrown when converting a Polars DataFrame to pandas) currently tells users to "pip install pyarrow"; update that recovery hint to point to the documented, version-pinned install instruction instead (e.g., direct users to the project's installation docs or to install the compatible wheel set such as the pinned polars/pyarrow versions used by this project) so users install the validated Arrow/Polars versions rather than a bare pyarrow; modify the string in the raise RuntimeError(...) inside the accessor to include the documented install command or specific pinned version reference.
20-20: Exercise the Arrow-backed path before depending on it by default.
use_pyarrow_extension_array=Truechanges the dtype backend thatPlotAccessorreceives, whilepyopenms_viz/_core.pystill does direct Series arithmetic and NumPy calls on those columns at Lines 595-600, 908-914, and 961-966. Please add a regression test that runsdf.ms_plot(...)through the relative-intensity/binning paths with nullable numeric Polars columns, or normalize the converted frame back to pandas/NumPy-friendly dtypes before dispatch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyopenms_viz/_polars_accessor.py` at line 20, The current conversion in _polars_accessor.py uses to_pandas(use_pyarrow_extension_array=True) which yields Arrow-backed dtypes that break numeric/NumPy operations in PlotAccessor/ms_plot and routines in pyopenms_viz/_core.py; change the conversion to produce NumPy-friendly dtypes (e.g., call to_pandas() without use_pyarrow_extension_array or cast nullable numeric columns to float/np.ndarray types) or explicitly normalize the resulting pandas_df columns back to pandas/NumPy-friendly dtypes before dispatch; additionally add a regression test that constructs a Polars DataFrame with nullable numeric columns and runs df.ms_plot(...) through the relative-intensity and binning paths to ensure the code in PlotAccessor/ms_plot and the arithmetic in pyopenms_viz/_core.py (around the relative-intensity/binning logic) works with those columns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pyopenms_viz/_polars_accessor.py`:
- Around line 22-25: The RuntimeError raised in pyopenms_viz._polars_accessor
(the message thrown when converting a Polars DataFrame to pandas) currently
tells users to "pip install pyarrow"; update that recovery hint to point to the
documented, version-pinned install instruction instead (e.g., direct users to
the project's installation docs or to install the compatible wheel set such as
the pinned polars/pyarrow versions used by this project) so users install the
validated Arrow/Polars versions rather than a bare pyarrow; modify the string in
the raise RuntimeError(...) inside the accessor to include the documented
install command or specific pinned version reference.
- Line 20: The current conversion in _polars_accessor.py uses
to_pandas(use_pyarrow_extension_array=True) which yields Arrow-backed dtypes
that break numeric/NumPy operations in PlotAccessor/ms_plot and routines in
pyopenms_viz/_core.py; change the conversion to produce NumPy-friendly dtypes
(e.g., call to_pandas() without use_pyarrow_extension_array or cast nullable
numeric columns to float/np.ndarray types) or explicitly normalize the resulting
pandas_df columns back to pandas/NumPy-friendly dtypes before dispatch;
additionally add a regression test that constructs a Polars DataFrame with
nullable numeric columns and runs df.ms_plot(...) through the relative-intensity
and binning paths to ensure the code in PlotAccessor/ms_plot and the arithmetic
in pyopenms_viz/_core.py (around the relative-intensity/binning logic) works
with those columns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 280be52f-14e7-4612-82fc-5a73c2107a89
📒 Files selected for processing (1)
pyopenms_viz/_polars_accessor.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/test_polars_accessor.py`:
- Around line 21-25: The test uses an unsupported backend name causing early
failure: change the backend argument in the ms_plot call from the accessor name
"ms_plotly" to the short backend id "plotly" so the backend resolver accepts it
(update the ms_plot invocation where it's called with backend="ms_plotly" to
backend="plotly" in test/test_polars_accessor.py).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d51ec01e-79d9-4d10-b52a-32b06c8b0584
📒 Files selected for processing (1)
test/test_polars_accessor.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
💡 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 3 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi @singjc, I’ve addressed the recent feedback from CodeRabbit and Copilot. Specifically, I've added unit test coverage for the new accessor, updated the error handling to gracefully raise an ImportError if pyarrow is missing, and ensured we are properly delegating to pandas.plot() to respect the library's entry points. The core CI tests (Ubuntu and Windows) are now fully passing. The only check failing is the Deploy Documentation preview, which i am pretty sure that it is some permission issue rather than a code error. Please let me know if you'd like any further adjustments! |
|
Hi @Sanjith1013, thanks for the PR. The implementation looks fine, it's unfortunate we can't use the same Also, for the added tests, I think we want to maybe also add some snapshot tests, to ensure we get the same output as with the pandas plotting. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fig = df_pl.ms_plot( | ||
| kind="spectrum", | ||
| x="m/z", | ||
| y="intensity", | ||
| backend="ms_plotly" | ||
| ) | ||
|
|
||
| assert fig is not None | ||
| assert type(fig).__name__ == "Figure" | ||
|
|
||
| def test_polars_vs_pandas_snapshot(): | ||
| """ | ||
| Test that Polars ms_plot() and Pandas plot() generate the exact same | ||
| underlying Plotly figure data (snapshot test). | ||
| """ | ||
| data = { | ||
| "m/z": [100.1, 100.2, 105.0, 105.1], | ||
| "intensity": [10.0, 50.0, 100.0, 90.0] | ||
| } | ||
|
|
||
| # Create identical DataFrames | ||
| df_pd = pd.DataFrame(data) | ||
| df_pl = pl.DataFrame(data) | ||
|
|
||
| # Generate figures | ||
| fig_pd = df_pd.plot(kind="spectrum", x="m/z", y="intensity", backend="ms_plotly") | ||
| fig_pl = df_pl.ms_plot(kind="spectrum", x="m/z", y="intensity", backend="ms_plotly") |
There was a problem hiding this comment.
These plot calls omit show_plot=False. In this codebase the backends call plot_obj.show() when show_plot is truthy (default True), which can open UI/renderers during CI and cause hangs/flakiness. Pass show_plot=False for both the Polars and pandas figure generation here (as done in the other plotting tests).
| # Compare the underlying JSON structure of the figures. | ||
| # This guarantees the Polars delegation yields 100% identical Plotly output. | ||
| assert fig_pd.to_json() == fig_pl.to_json() |
There was a problem hiding this comment.
fig_pd.to_json() == fig_pl.to_json() is a very brittle assertion: Plotly JSON serialization can vary due to key ordering, float formatting, or default metadata (even if the figures are semantically identical). Consider comparing parsed JSON objects (e.g., json.loads(...)) using the same recursive/tolerant comparison already implemented in pyopenms_viz.testing.PlotlySnapshotExtension.compare_json, or otherwise normalize/remove UID-like fields before comparing.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # 1. Safety Catch: Use Arrow-backed arrays with a clear error fallback | ||
| pandas_df = self._df.to_pandas(use_pyarrow_extension_array=True) |
There was a problem hiding this comment.
to_pandas(use_pyarrow_extension_array=True) produces Arrow extension dtypes in pandas. Downstream code in this repo uses x.dtype.kind (e.g., numeric-vs-categorical branching), which can raise on extension dtypes and break plotting for Polars inputs. Consider omitting use_pyarrow_extension_array=True by default (or normalizing dtypes after conversion) unless the core pipeline is updated to use pandas.api.types checks instead of dtype.kind.
| # 1. Safety Catch: Use Arrow-backed arrays with a clear error fallback | |
| pandas_df = self._df.to_pandas(use_pyarrow_extension_array=True) | |
| # 1. Safety Catch: Convert to a pandas DataFrame using default dtypes | |
| pandas_df = self._df.to_pandas() |
| # Create identical DataFrames | ||
| df_pd = pd.DataFrame(data) | ||
| df_pl = pl.DataFrame(data) |
There was a problem hiding this comment.
Comparing Figure.to_json() strings is unnecessarily brittle because JSON object key ordering and serialization formatting aren’t guaranteed to be stable. To avoid flaky tests, compare parsed JSON structures (e.g., json.loads(...)) or use the existing Plotly snapshot comparison helper used elsewhere in this repo.
|
Hi @singjc, Thanks for the review and the green light on the implementation! I completely agree that the syntax difference isn't ideal. I originally explored using .plot() for Polars, but since Polars recently introduced its own native .plot property (powered by Altair), trying to use @pl.api.register_dataframe_namespace("plot") causes a namespace conflict, and monkey-patching pl.DataFrame.plot directly felt too invasive and it might break other user integrations. A potential idea for API unification: Since we can't easily alias Polars to .plot, we could go the other way and register .ms_plot as a standard Pandas DataFrame accessor as well (using @pd.api.extensions.register_dataframe_accessor). This would leave the existing Pandas .plot backend completely intact for backward compatibility, but allow you to tell users: "Just use df.ms_plot() regardless of whether you are using Pandas or Polars!" Let me know if you'd like me to add that! In the meantime, I have pushed the updates you requested: Added a strict snapshot test (test_polars_vs_pandas_snapshot) that generates a figure using Pandas .plot(), generates a second using Polars .ms_plot(), and asserts that their underlying Plotly JSON structures are 100% identical. Updated the README.md with a clear callout explaining the .ms_plot syntax requirement for Polars so users aren't confused. Let me know if you'd like any further tweaks! |
|
Hi @singjc, |
|
@Sanjith1013, I like the idea of registering a separate namespace in pandas (similar to geopandas). I was thinking about this previously, but never got around to looking into it. This would also I think integrate nicely with pyopenms. If you're interested you could look into this in a separate PR to see if it's possible. For the snapshot test, I meant using syrupy, which is the current testing framework we use for testing the stability of the generated plots. Take a look at the current existing tests to see how they are done |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi @singjc, That sounds like a great plan! I would love to tackle registering the .ms_plot namespace for Pandas in a separate PR to keep the API fully unified. I'll get started on that right after this! I have updated the tests to use syrupy for the snapshot comparison as requested. I replaced the manual JSON assertion with the snapshot fixture and pushed the generated snapshot files. I also just pushed a quick fix to address the latest bot feedback: I removed the unused imports and fixed the global state leak by properly scoping the pd.options.plotting.backend override to the load_backend fixture. The syrupy snapshots are cleanly generated now without breaking the conftest.py setup! Let me know if everything looks good to merge! |
|
Hi @singjc, |
|
Hi @Sanjith1013, thanks for adding the syrupy snapshot test. I have been thinking a bit more about this, and I'm wondering if this is the right approach / if it's worth trying to add support for polars. In this approach, you basically create an interface to generate MS plots using polars dfs through an addition Let's see what @jcharkow thinks as well. |
|
My worry with this approach is that since polars dataframes tend to be a lot more memory efficient than pandas, by converting to polars it may lead to an unexpected memory consumption since they are not expecting the conversion. This is likely better if pandas is using arrow internally which I have seen that you have done but this does add additional package complexity. For me personally, I am a bit hesitant on this. Besides calling |
|
Thank you both for the thoughtful reviews. You make excellent architectural points, and I completely agree with your hesitation. Hiding a .to_pandas() conversion inside an accessor acts as a silent memory landmine. As @jcharkow noted, I used PyArrow extension arrays (df.to_pandas(use_pyarrow_extension_array=True)) to try and achieve a zero-copy conversion and avoid that initial allocation overhead,But in the moment that PyArrow-backed dataframe is passed into our downstream plotting backends (Plotly/Matplotlib), those libraries will eventually cast to NumPy arrays to render the coordinates, forcing a full memory materialization anyway. A user processing a massive top-down dataset in Polars specifically to avoid OOM errors would still hit a sudden RAM spike the second they trigger the plot. Saving the user from explicitly typing df.to_pandas().plot() is definitely not worth that risk, nor is it worth the added package complexity of injecting polars and pyarrow dependencies into the core library. True workarounds for this—such as server-side data decimation (e.g., LTTB), abstract rendering (Datashader), or rewriting the plotting backend to use WebSockets and custom WebGL (deck.gl)—would require massive architectural shifts that are entirely out of scope for a simple accessor. I think closing this PR to keep the codebase lightweight and protect the library's memory integrity would be the best option. Would you like me to submit a much smaller PR that simply adds a short "Polars Integration" snippet to the README/Documentation, showing users the explicit and optimal way to pass Polars data to the plotting backend instead? |
|
Yes I think adding polars to the documentation would be a good idea. Probably could add it in the getting started notebook. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
docs/Getting Started.ipynb:1
- This guidance recommends explicitly converting to pandas before calling the
.ms_plotaccessor, but.ms_plotis implemented as a Polars accessor that itself performs conversion and then delegates to pandas plotting. Suggest updating the text to either (a) recommend callingdf_polars.ms_plot(...)directly (letting the accessor handle conversion), or (b) recommend explicit conversion and then calling the pandas.plot(...)API—right now the narrative mixes both and may confuse users.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| init | ||
| """ | ||
|
|
||
| from . import _polars_accessor # Import for side effects: registers pandas accessors. noqa: F401 |
There was a problem hiding this comment.
The inline suppression is unlikely to be recognized by common linters because it uses noqa: inside a longer comment rather than # noqa: F401 at the end of the line. Also, this import registers a Polars namespace accessor (not a pandas accessor), so the comment is misleading. Suggest updating the comment and placing a proper # noqa: F401 marker at the end of the line (or removing it if F401 is not enforced).
| from . import _polars_accessor # Import for side effects: registers pandas accessors. noqa: F401 | |
| from . import _polars_accessor # Import for side effects: registers Polars namespace accessor. # noqa: F401 |
|
|
||
| # Skip these tests if the user doesn't have Polars or PyArrow installed | ||
| pl = pytest.importorskip("polars") | ||
| pytest.importorskip("pyarrow") |
There was a problem hiding this comment.
PEP8/test style: top-level declarations should be separated by a blank line. Add a blank line between the importorskip(...) calls and the @pytest.fixture decorator to keep formatting consistent and avoid style/lint failures.
| pytest.importorskip("pyarrow") | |
| pytest.importorskip("pyarrow") |
| ) | ||
|
|
||
| assert fig is not None | ||
| assert type(fig).__name__ == "Figure" |
There was a problem hiding this comment.
Asserting on type(fig).__name__ is brittle and can break if the backend returns a subclass/proxy (or if the backend changes implementation while still being a valid Plotly figure). Prefer asserting against the expected Plotly figure type (when available) or validating key structural properties (e.g., hasattr(fig, 'to_plotly_json'), presence of expected traces/layout).
| "template": { | ||
| "data": { |
There was a problem hiding this comment.
The snapshot includes the full Plotly layout.template, which is very large and tends to change across Plotly versions/themes, making the snapshot brittle and noisy to update. Consider normalizing the figure before snapshotting (e.g., stripping layout.template and other auto-populated defaults) so the snapshot focuses on the plot’s meaningful semantics (traces, titles, axes ranges/labels).
hi @jcharkow , |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Switch Sphinx Gallery Binder branch target from gh_pages to main. Fixes OpenMS#94
Sanjith1013
force-pushed
the
feat-polars-support
branch
from
4baee6a to
03190d0
Compare
Resolves #55
Technical Overview
Namespace Extension: Registered a custom ms_plot namespace for Polars DataFrames to avoid collisions with Polars' internal .plot (Altair) API.
Safe Delegation: Implemented an upfront conversion to Pandas via pyarrow within the accessor. This ensures full compatibility with the existing BasePlot architecture and legacy Pandas-specific manipulations.
Consistent Style: Updated requirements.txt with version locks (polars==1.38.1, pyarrow==23.0.1) following the project's existing formatting.
Dynamic Dependencies: Utilized a try-except wrapper in _polars_accessor.py to ensure Polars remains an optional dependency.
Summary by CodeRabbit
New Features
Tests
Chores