Merged
Conversation
- add ScatterPlotWidget, BarPlotWidget, and HistogramWidget with shared series normalization - wire new widgets into exports and anywidget frontend mapping, plus notebook/marimo/vscode demos - harden trajectory dict validation for empty species lists and extend widget tests
|
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 three new plotting widgets (ScatterPlot, BarPlot, Histogram), JSON-safe plotting normalization helpers, TypeScript renderers, Marimo-aware MatterViz asset handling and caching, expanded example demos/notebooks and MIME examples, trajectory species-list validation, tests for new behavior, and a local pre-commit marimo check. Changes
Sequence DiagramsequenceDiagram
participant User
participant PyWidget as ScatterPlotWidget\n(Python)
participant Normalize as normalize_plot_series\n& normalize_plot_json
participant FrontEnd as anywidget.ts\n(TypeScript Renderer)
participant UI as ScatterPlot Component\n(matterviz)
User->>PyWidget: Instantiate widget with series & options
PyWidget->>Normalize: Validate & normalize series, axes, styles
Normalize-->>PyWidget: Normalized JSON-safe payload
PyWidget->>FrontEnd: Sync widget_type & props over comm
FrontEnd->>FrontEnd: Select scatter_plot renderer
FrontEnd->>UI: Mount ScatterPlot with normalized props
UI-->>User: Render interactive plot
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/widgets/jupyter_demo.ipynb (1)
488-492:⚠️ Potential issue | 🟡 MinorUse explicit
display(...)for MIME demo objects in this linted notebook.These expression statements are flagged by Ruff (B018). Wrapping them in
display(...)preserves notebook behavior and avoids lint noise.🔧 Suggested fix
+from IPython.display import display ... -ase_atoms +display(ase_atoms) ... -ase_molecule +display(ase_molecule)Also applies to: 510-514
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/widgets/jupyter_demo.ipynb` around lines 488 - 492, Replace bare expression statements that rely on Jupyter MIME rendering (the variable ase_atoms) with explicit display() calls: locate the expression statements that simply reference ase_atoms (and the similar occurrences at lines noted in the comment) and wrap them in display(ase_atoms). Update any imports or ensure from IPython.display import display is present in the notebook if not already; adjust the statements in the cells where ase_atoms is created/printed (references: ase_atoms) to use display(ase_atoms) to satisfy the linter and preserve notebook output.
🧹 Nitpick comments (3)
examples/widgets/vscode_interactive_demo.py (1)
29-30: Prefer instance.display()overMatterVizWidget.display(widget)and drop the internal import.The current class-style invocation works, but instance calls are clearer and avoid coupling this example to
pymatviz.widgets.matterviz.♻️ Suggested refactor
-from pymatviz.widgets.matterviz import MatterVizWidget ... -MatterVizWidget.display(scatter_plot_widget) +scatter_plot_widget.display() ... -MatterVizWidget.display(bar_plot_widget) +bar_plot_widget.display() ... -MatterVizWidget.display(histogram_widget) +histogram_widget.display()Run this read-only check; expected result after refactor: no matches.
#!/bin/bash rg -n --type=py 'from pymatviz\.widgets\.matterviz import MatterVizWidget|MatterVizWidget\.display\(' examples/widgetsAlso applies to: 198-220, 248-248
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/widgets/vscode_interactive_demo.py` around lines 29 - 30, Replace the class-style invocation and internal import for MatterVizWidget with an instance call: remove the line importing MatterVizWidget from pymatviz.widgets.matterviz and any use of MatterVizWidget.display(widget) and instead construct the widget instance (e.g., widget = MatterVizWidget(...)) where used and call widget.display(); update all occurrences referenced (the current import at top and usages around lines 29 and the other ranges 198-220 and 248) so the example uses the instance method and no longer imports the internal module.pymatviz/widgets/bar_plot.py (1)
13-82: Consider extendingstate_fieldsforto_dict()completeness.The base
MatterVizWidget.to_dict()method returns only fields listed instate_fields(currently"widget_type","style","show_controls"). Ifto_dict()should serialize widget-specific fields likeseries,orientation,mode, etc., you'll need to overridestate_fieldsin this class.If
to_dict()isn't intended for serializing widget-specific state, this is fine as-is.💡 Optional: extend state_fields if needed
class BarPlotWidget(MatterVizWidget): """MatterViz widget wrapper for grouped/stacked/overlay bar plots.""" + state_fields: tuple[str, ...] = ( + "widget_type", "style", "show_controls", + "series", "orientation", "mode", "x_axis", "y_axis", "y2_axis", + "display", "legend", "bar", "line", "ref_lines", "controls", + ) + series = tl.List(allow_none=True).tag(sync=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pymatviz/widgets/bar_plot.py` around lines 13 - 82, The BarPlotWidget currently doesn't include its widget-specific traits in serialization because MatterVizWidget.to_dict() only exports names in state_fields; override the class attribute state_fields in BarPlotWidget (e.g., set state_fields = super().state_fields + ("series", "orientation", "mode", "x_axis", "y_axis", "y2_axis", "display", "legend", "bar", "line", "ref_lines", "controls")) so to_dict() will include those trait names; ensure the names you add exactly match the trait names (series, orientation, mode, x_axis, y_axis, y2_axis, display, legend, bar, line, ref_lines, controls) and keep it a sequence type the base class expects.examples/widgets/marimo_demo.py (1)
202-224: Minor: Avoid redundantnp.linspacecalls.
np.linspace(0, 6.0, 60)is computed twice per series (once forx, once innp.sin/np.cos). Consider storing it in a variable for clarity and slight efficiency.♻️ Optional cleanup
`@app.cell` def _(np, pmv): + x_vals = np.linspace(0, 6.0, 60).tolist() scatter_series = [ { "label": "sin(x)", - "x": np.linspace(0, 6.0, 60).tolist(), - "y": np.sin(np.linspace(0, 6.0, 60)).tolist(), + "x": x_vals, + "y": np.sin(np.linspace(0, 6.0, 60)).tolist(), }, { "label": "cos(x)", - "x": np.linspace(0, 6.0, 60).tolist(), - "y": np.cos(np.linspace(0, 6.0, 60)).tolist(), + "x": x_vals, + "y": np.cos(np.linspace(0, 6.0, 60)).tolist(), "y_axis": "y2", }, ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/widgets/marimo_demo.py` around lines 202 - 224, Compute np.linspace(0, 6.0, 60) once and reuse it instead of calling it multiple times: create a variable (e.g., x_vals) before building scatter_series, use x_vals for each series' "x" and pass np.sin(x_vals) / np.cos(x_vals) for the "y" values; update the code around scatter_series and the subsequent pmv.ScatterPlotWidget call to reference that variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@examples/widgets/jupyter_demo.ipynb`:
- Around line 488-492: Replace bare expression statements that rely on Jupyter
MIME rendering (the variable ase_atoms) with explicit display() calls: locate
the expression statements that simply reference ase_atoms (and the similar
occurrences at lines noted in the comment) and wrap them in display(ase_atoms).
Update any imports or ensure from IPython.display import display is present in
the notebook if not already; adjust the statements in the cells where ase_atoms
is created/printed (references: ase_atoms) to use display(ase_atoms) to satisfy
the linter and preserve notebook output.
---
Nitpick comments:
In `@examples/widgets/marimo_demo.py`:
- Around line 202-224: Compute np.linspace(0, 6.0, 60) once and reuse it instead
of calling it multiple times: create a variable (e.g., x_vals) before building
scatter_series, use x_vals for each series' "x" and pass np.sin(x_vals) /
np.cos(x_vals) for the "y" values; update the code around scatter_series and the
subsequent pmv.ScatterPlotWidget call to reference that variable.
In `@examples/widgets/vscode_interactive_demo.py`:
- Around line 29-30: Replace the class-style invocation and internal import for
MatterVizWidget with an instance call: remove the line importing MatterVizWidget
from pymatviz.widgets.matterviz and any use of MatterVizWidget.display(widget)
and instead construct the widget instance (e.g., widget = MatterVizWidget(...))
where used and call widget.display(); update all occurrences referenced (the
current import at top and usages around lines 29 and the other ranges 198-220
and 248) so the example uses the instance method and no longer imports the
internal module.
In `@pymatviz/widgets/bar_plot.py`:
- Around line 13-82: The BarPlotWidget currently doesn't include its
widget-specific traits in serialization because MatterVizWidget.to_dict() only
exports names in state_fields; override the class attribute state_fields in
BarPlotWidget (e.g., set state_fields = super().state_fields + ("series",
"orientation", "mode", "x_axis", "y_axis", "y2_axis", "display", "legend",
"bar", "line", "ref_lines", "controls")) so to_dict() will include those trait
names; ensure the names you add exactly match the trait names (series,
orientation, mode, x_axis, y_axis, y2_axis, display, legend, bar, line,
ref_lines, controls) and keep it a sequence type the base class expects.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
examples/widgets/jupyter_demo.ipynbexamples/widgets/marimo_demo.pyexamples/widgets/vscode_interactive_demo.pypymatviz/__init__.pypymatviz/widgets/__init__.pypymatviz/widgets/_normalize.pypymatviz/widgets/bar_plot.pypymatviz/widgets/histogram.pypymatviz/widgets/scatter_plot.pypymatviz/widgets/trajectory.pypymatviz/widgets/web/anywidget.tstests/widgets/test_new_widgets.pytests/widgets/test_trajectory_widget.py
Marimo serializes cell outputs independently with a ~10 MB limit, causing widgets embedding ~10 MB of inline JS to fail. anywidget also only treats _esm values starting with http(s):// as URLs. - Serve ESM via marimo's virtual-file system with absolute URL resolution so anywidget imports it as a URL (not inline source) - Keep CSS inline (~166 KB) to avoid stylesheet loading edge cases - Add _in_marimo_runtime, _marimo_esm_url, _init_marimo_assets with 27 tests including mutation-verified coverage - Fix marimo demo cell dependency wiring (_struct, _phonon_doc, _band_data, _dos_data), broken markdown cells, and ruff noqa - Add marimo check --strict pre-commit hook - Fix RUF005 in bar_plot.py, TRY300 in matterviz.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
examples/widgets/vscode_interactive_demo.py (1)
198-248: Minor inconsistency in display patterns (optional).Some widgets use
display(widget)from IPython while others usewidget.display(). Both work, but usingwidget.display()consistently would align with the rest of the file and enable method chaining.♻️ Optional: Consistent display() usage
-display(scatter_plot_widget) +scatter_plot_widget.display()-display(bar_plot_widget) +bar_plot_widget.display()-display(histogram_widget) +histogram_widget.display()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/widgets/vscode_interactive_demo.py` around lines 198 - 248, Change the inconsistent IPython display calls to use the widget instance display method for consistency and chaining: replace display(scatter_plot_widget) with scatter_plot_widget.display(), replace display(bar_plot_widget) with bar_plot_widget.display(), and replace display(histogram_widget) with histogram_widget.display(); keep all widget construction (e.g., scatter_plot_widget, bar_plot_widget, histogram_widget and their argument names like series, mode, style) unchanged and ensure you import or rely on the widget.display() method already provided by the pmv widget classes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@examples/widgets/vscode_interactive_demo.py`:
- Around line 198-248: Change the inconsistent IPython display calls to use the
widget instance display method for consistency and chaining: replace
display(scatter_plot_widget) with scatter_plot_widget.display(), replace
display(bar_plot_widget) with bar_plot_widget.display(), and replace
display(histogram_widget) with histogram_widget.display(); keep all widget
construction (e.g., scatter_plot_widget, bar_plot_widget, histogram_widget and
their argument names like series, mode, style) unchanged and ensure you import
or rely on the widget.display() method already provided by the pmv widget
classes.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.pre-commit-config.yamlexamples/widgets/jupyter_demo.ipynbexamples/widgets/marimo_demo.pyexamples/widgets/vscode_interactive_demo.pypymatviz/widgets/bar_plot.pypymatviz/widgets/matterviz.pytests/widgets/test_matterviz.py
Replace synthetic band/DOS demo payloads with phonon test fixtures and add concise explanatory headings across notebook, marimo, and VSCode interactive demos.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
examples/widgets/jupyter_demo.ipynb (1)
58-74: Avoid committing transient widget outputs in this notebook.This output block includes runtime-only values (
model_id, object memory repr) that will churn on every re-run and make diffs noisy. Please clear widget outputs before committing notebook changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/widgets/jupyter_demo.ipynb` around lines 58 - 74, The notebook contains runtime-only widget outputs (e.g., the application/vnd.jupyter.widget-view+json block with keys like "model_id" and the printed "<pymatviz.widgets.convex_hull.ConvexHullWidget...>"), which churn diffs; remove/clear these transient outputs before committing by clearing widget outputs for this cell (or run a notebook-cleaning step such as nbstripout/nbconvert to strip widget state), ensuring the ConvexHullWidget output and its model_id/version fields are removed from the saved .ipynb.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@examples/widgets/jupyter_demo.ipynb`:
- Around line 58-74: The notebook contains runtime-only widget outputs (e.g.,
the application/vnd.jupyter.widget-view+json block with keys like "model_id" and
the printed "<pymatviz.widgets.convex_hull.ConvexHullWidget...>"), which churn
diffs; remove/clear these transient outputs before committing by clearing widget
outputs for this cell (or run a notebook-cleaning step such as
nbstripout/nbconvert to strip widget state), ensuring the ConvexHullWidget
output and its model_id/version fields are removed from the saved .ipynb.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/widgets/jupyter_demo.ipynbexamples/widgets/marimo_demo.pyexamples/widgets/vscode_interactive_demo.py
- Skip js() virtual file call when virtual_files_supported=False to prevent ~14 MB base64 data URL allocations that crash the kernel - Fall back to relative ./@file/ URL when request context is unavailable - Set resolved ESM URL on class (not per-instance) to avoid N copies - Use display() from IPython for plot widgets that shadow the method - Document VS Code extension limitation in MatterVizWidget docstring
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/widgets/test_matterviz.py (2)
342-374: Consider adding test forvirtual_files_supported=False.The tests cover the case where marimo is not installed and various fallback scenarios, but there's no explicit test for when
get_context().virtual_files_supportedreturnsFalse(the VS Code extension case mentioned in the implementation). This is an important edge case that triggers the earlyreturn Noneat line 47 of matterviz.py.🧪 Suggested test case
def test_marimo_esm_url_vfiles_not_supported() -> None: """Returns None when virtual files aren't supported (VS Code extension).""" mock_ctx = MagicMock() mock_ctx.virtual_files_supported = False mock_ctx_mod = MagicMock() mock_ctx_mod.get_context.return_value = mock_ctx mods = { "marimo._output.data.data": MagicMock(), "marimo._runtime.context": mock_ctx_mod, } with patch.dict("sys.modules", mods): assert _marimo_esm_url("code") is None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/widgets/test_matterviz.py` around lines 342 - 374, Add a unit test that covers the case where marimo reports virtual files are not supported: mock the marimo._runtime.context module so get_context() returns an object with virtual_files_supported = False, include a marimo._output.data.data MagicMock as well, patch sys.modules with these mods and assert that calling _marimo_esm_url("code") returns None; this exercise targets the early return in the _marimo_esm_url function when get_context().virtual_files_supported is False.
59-71: Potential incomplete cleanup if attributes are newly created during test.If
_esmor_csswere not set before the test (saved_esm is None) but get set during the test, lines 68-71 won't clean them up. This could leak state to subsequent tests.Consider unconditionally deleting class-level
_esm/_cssattributes at teardown and only restoring if they existed:🔧 Proposed fix for complete cleanup
`@pytest.fixture` def _clean_asset_cache() -> Generator[None]: """Save and restore MatterVizWidget class-level asset state around a test.""" saved_esm = getattr(matterviz.MatterVizWidget, "_esm", None) saved_css = getattr(matterviz.MatterVizWidget, "_css", None) + had_esm = hasattr(matterviz.MatterVizWidget, "_esm") + had_css = hasattr(matterviz.MatterVizWidget, "_css") saved_cache = matterviz.MatterVizWidget._asset_cache.copy() matterviz.MatterVizWidget._asset_cache.clear() yield matterviz.MatterVizWidget._asset_cache = saved_cache - if saved_esm is not None: + # Clean up any newly created attributes first + if hasattr(matterviz.MatterVizWidget, "_esm"): + delattr(matterviz.MatterVizWidget, "_esm") + if hasattr(matterviz.MatterVizWidget, "_css"): + delattr(matterviz.MatterVizWidget, "_css") + # Restore original attributes if they existed + if had_esm: matterviz.MatterVizWidget._esm = saved_esm - if saved_css is not None: + if had_css: matterviz.MatterVizWidget._css = saved_css🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/widgets/test_matterviz.py` around lines 59 - 71, The fixture _clean_asset_cache leaves class-level attributes _esm and _css if they are created during a test because it only restores them when saved_esm/saved_css are not None; update the teardown to restore previous values when saved_esm/saved_css exist and otherwise remove any attributes added by the test (use delattr on matterviz.MatterVizWidget for "_esm" and "_css" when saved_* is None), and ensure _asset_cache is restored by assigning saved_cache back; implement this logic in the _clean_asset_cache fixture so newly created _esm/_css are cleaned up while existing ones are restored.pymatviz/widgets/matterviz.py (1)
204-223: Redundant CSS assignment and class-level state mutation concern.Line 223 assigns
css_texttoself.__class__._css, but this is already set by_set_class_assets()on line 216. The assignment is a no-op.Additionally, mutating
_esmat the class level (line 221) means subsequent non-marimo initializations (or subclasses) would inherit the URL string instead of the ESM source. IfMatterVizWidgetis subclassed, the subclass would share the same_asset_cacheand could inherit a URL string where ESM content is expected.Consider whether these semantics are intentional for shared widget instances.
🔧 Proposed cleanup for redundant assignment
esm_url = _marimo_esm_url(esm_text) if esm_url: self.__class__._esm = esm_url - # CSS inline on class — shared across instances - self.__class__._css = css_text + # CSS already set by _set_class_assets() — no action needed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pymatviz/widgets/matterviz.py` around lines 204 - 223, The _init_marimo_assets method redundantly reassigns class _css (no-op after _set_class_assets()) and dangerously mutates class _esm with a URL string; instead remove the redundant self.__class__._css = css_text line and avoid writing back to the class _esm. Keep class attribute _esm as the original source set by _set_class_assets(), and store the marimo-serving URL on the instance (e.g. self._esm_url or another instance attribute) so only this widget instance uses the URL; update any code that consumes the ESM to prefer the instance attribute (falling back to self.__class__._esm) and leave _set_class_assets, _esm and _css unchanged at class scope.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pymatviz/widgets/matterviz.py`:
- Around line 204-223: The _init_marimo_assets method redundantly reassigns
class _css (no-op after _set_class_assets()) and dangerously mutates class _esm
with a URL string; instead remove the redundant self.__class__._css = css_text
line and avoid writing back to the class _esm. Keep class attribute _esm as the
original source set by _set_class_assets(), and store the marimo-serving URL on
the instance (e.g. self._esm_url or another instance attribute) so only this
widget instance uses the URL; update any code that consumes the ESM to prefer
the instance attribute (falling back to self.__class__._esm) and leave
_set_class_assets, _esm and _css unchanged at class scope.
In `@tests/widgets/test_matterviz.py`:
- Around line 342-374: Add a unit test that covers the case where marimo reports
virtual files are not supported: mock the marimo._runtime.context module so
get_context() returns an object with virtual_files_supported = False, include a
marimo._output.data.data MagicMock as well, patch sys.modules with these mods
and assert that calling _marimo_esm_url("code") returns None; this exercise
targets the early return in the _marimo_esm_url function when
get_context().virtual_files_supported is False.
- Around line 59-71: The fixture _clean_asset_cache leaves class-level
attributes _esm and _css if they are created during a test because it only
restores them when saved_esm/saved_css are not None; update the teardown to
restore previous values when saved_esm/saved_css exist and otherwise remove any
attributes added by the test (use delattr on matterviz.MatterVizWidget for
"_esm" and "_css" when saved_* is None), and ensure _asset_cache is restored by
assigning saved_cache back; implement this logic in the _clean_asset_cache
fixture so newly created _esm/_css are cleaned up while existing ones are
restored.
Use dedicated class-level marimo URL cache fields instead of overloading the generic asset tuple cache, and keep test cache cleanup aligned with the new cache state.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/widgets/test_matterviz.py (1)
28-32: Consider using a custom exception class for cleaner import blocking.The static analysis tool flags TRY003 for the long message in the exception. While this is a minor issue in test code, you could simplify:
♻️ Optional refactor
def _block_marimo(name: str, *args: Any, **kwargs: Any) -> types.ModuleType: """Import hook that raises ImportError for any marimo submodule.""" if name.startswith("marimo"): - raise ImportError(f"No module named '{name}'") + raise ImportError(name) return _real_import(name, *args, **kwargs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/widgets/test_matterviz.py` around lines 28 - 32, The test import hook _block_marimo currently raises ImportError with a long formatted message which triggers TRY003; create a small custom exception (e.g., MarimoImportBlocked) and raise that from _block_marimo when name.startswith("marimo") (or replace the long ImportError message with a minimal message) so static analysis stops flagging the long string—update any test code that expects ImportError to catch the new custom exception or adjust assertions accordingly.pymatviz/widgets/matterviz.py (1)
164-168: Class-level Marimo URL cache is never reset across different ESM content.The
_marimo_esm_cache_readyflag prevents re-computing the URL even if_esmcontent changes (e.g., after a version override in a prior widget). If a first widget is created withoutversion_overrideand caches the URL, then subsequent widgets withversion_overridewill skip the Marimo path entirely (due to theif version_overridebranch), which is fine. However, if the class-level_esmcontent were ever to change between calls to_init_marimo_assets, the stale cached URL would be used.This is likely acceptable given the current initialization flow (version_override bypasses Marimo path), but consider adding a comment clarifying this assumption.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pymatviz/widgets/matterviz.py` around lines 164 - 168, The class-level Marimo URL cache (_marimo_esm_cached_url and _marimo_esm_cache_ready) can become stale if the class-level _esm content changes between calls to _init_marimo_assets; either reset the cache when _esm changes or at minimum document the assumption that _esm never changes after first initialization. Update _init_marimo_assets (and any place that may modify _esm or call it) to compare the current _esm to the value used for caching and clear _marimo_esm_cached_url/_marimo_esm_cache_ready when they differ, or add a concise comment near the class variables and in _init_marimo_assets referencing version_override and the single-initialization assumption to make the behavior explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pymatviz/widgets/matterviz.py`:
- Around line 164-168: The class-level Marimo URL cache (_marimo_esm_cached_url
and _marimo_esm_cache_ready) can become stale if the class-level _esm content
changes between calls to _init_marimo_assets; either reset the cache when _esm
changes or at minimum document the assumption that _esm never changes after
first initialization. Update _init_marimo_assets (and any place that may modify
_esm or call it) to compare the current _esm to the value used for caching and
clear _marimo_esm_cached_url/_marimo_esm_cache_ready when they differ, or add a
concise comment near the class variables and in _init_marimo_assets referencing
version_override and the single-initialization assumption to make the behavior
explicit.
In `@tests/widgets/test_matterviz.py`:
- Around line 28-32: The test import hook _block_marimo currently raises
ImportError with a long formatted message which triggers TRY003; create a small
custom exception (e.g., MarimoImportBlocked) and raise that from _block_marimo
when name.startswith("marimo") (or replace the long ImportError message with a
minimal message) so static analysis stops flagging the long string—update any
test code that expects ImportError to catch the new custom exception or adjust
assertions accordingly.
Replace manual state_fields tuples with automatic discovery via self.traits(sync=True) in MatterVizWidget.to_dict(). This fixes serialization for ScatterPlotWidget and HistogramWidget which were missing their state_fields definitions, and eliminates the need for every subclass to maintain a redundant field list. Also simplify production code (DRY numpy conversion in normalize_plot_json, loop-ify lattice params, remove narrating comments in trajectory.py) and split test_new_widgets.py into test_normalize.py, test_widget_construction.py, and test_mime.py organized by domain rather than chronology.
- Rename MatterVizWidget.display() to show() to avoid shadowing the 'display' traitlet on ScatterPlot/BarPlot/HistogramWidget which made widget.display() raise TypeError: 'dict' object is not callable - Add configure_assets(version=, esm_src=, css_src=) for global asset override via version tags, HTTP URLs, or local file paths - Fix marimo demo: cells now properly return and declare dependencies for _struct, _band_data, _phonon_doc, _dos_data - Simplify matterviz.py: flatten _marimo_esm_url resolution, trim docstrings, merge isinstance checks in normalize_plot_json
Runs all three widget demos (vscode, marimo, jupyter) in a single consolidated step to catch runtime errors in demo scripts.
janosh
added a commit
that referenced
this pull request
Mar 1, 2026
* Add generic plot widgets and demos - add ScatterPlotWidget, BarPlotWidget, and HistogramWidget with shared series normalization - wire new widgets into exports and anywidget frontend mapping, plus notebook/marimo/vscode demos - harden trajectory dict validation for empty species lists and extend widget tests * Fix marimo widget rendering and improve demo notebooks Marimo serializes cell outputs independently with a ~10 MB limit, causing widgets embedding ~10 MB of inline JS to fail. anywidget also only treats _esm values starting with http(s):// as URLs. - Serve ESM via marimo's virtual-file system with absolute URL resolution so anywidget imports it as a URL (not inline source) - Keep CSS inline (~166 KB) to avoid stylesheet loading edge cases - Add _in_marimo_runtime, _marimo_esm_url, _init_marimo_assets with 27 tests including mutation-verified coverage - Fix marimo demo cell dependency wiring (_struct, _phonon_doc, _band_data, _dos_data), broken markdown cells, and ruff noqa - Add marimo check --strict pre-commit hook - Fix RUF005 in bar_plot.py, TRY300 in matterviz.py * Update widget demos with realistic fixture data and clearer narratives Replace synthetic band/DOS demo payloads with phonon test fixtures and add concise explanatory headings across notebook, marimo, and VSCode interactive demos. * Guard against marimo VS Code extension kernel crash - Skip js() virtual file call when virtual_files_supported=False to prevent ~14 MB base64 data URL allocations that crash the kernel - Fall back to relative ./@file/ URL when request context is unavailable - Set resolved ESM URL on class (not per-instance) to avoid N copies - Use display() from IPython for plot widgets that shadow the method - Document VS Code extension limitation in MatterVizWidget docstring * Simplify marimo ESM caching in MatterVizWidget Use dedicated class-level marimo URL cache fields instead of overloading the generic asset tuple cache, and keep test cache cleanup aligned with the new cache state. * Auto-discover to_dict fields from synced traitlets and reorganize tests Replace manual state_fields tuples with automatic discovery via self.traits(sync=True) in MatterVizWidget.to_dict(). This fixes serialization for ScatterPlotWidget and HistogramWidget which were missing their state_fields definitions, and eliminates the need for every subclass to maintain a redundant field list. Also simplify production code (DRY numpy conversion in normalize_plot_json, loop-ify lattice params, remove narrating comments in trajectory.py) and split test_new_widgets.py into test_normalize.py, test_widget_construction.py, and test_mime.py organized by domain rather than chronology. * Rename display() to show(), add configure_assets(), fix marimo cell deps - Rename MatterVizWidget.display() to show() to avoid shadowing the 'display' traitlet on ScatterPlot/BarPlot/HistogramWidget which made widget.display() raise TypeError: 'dict' object is not callable - Add configure_assets(version=, esm_src=, css_src=) for global asset override via version tags, HTTP URLs, or local file paths - Fix marimo demo: cells now properly return and declare dependencies for _struct, _band_data, _phonon_doc, _dos_data - Simplify matterviz.py: flatten _marimo_esm_url resolution, trim docstrings, merge isinstance checks in normalize_plot_json * Add CI job to execute widget demos Runs all three widget demos (vscode, marimo, jupyter) in a single consolidated step to catch runtime errors in demo scripts.
janosh
added a commit
that referenced
this pull request
Mar 2, 2026
* Add generic plot widgets and demos - add ScatterPlotWidget, BarPlotWidget, and HistogramWidget with shared series normalization - wire new widgets into exports and anywidget frontend mapping, plus notebook/marimo/vscode demos - harden trajectory dict validation for empty species lists and extend widget tests * Fix marimo widget rendering and improve demo notebooks Marimo serializes cell outputs independently with a ~10 MB limit, causing widgets embedding ~10 MB of inline JS to fail. anywidget also only treats _esm values starting with http(s):// as URLs. - Serve ESM via marimo's virtual-file system with absolute URL resolution so anywidget imports it as a URL (not inline source) - Keep CSS inline (~166 KB) to avoid stylesheet loading edge cases - Add _in_marimo_runtime, _marimo_esm_url, _init_marimo_assets with 27 tests including mutation-verified coverage - Fix marimo demo cell dependency wiring (_struct, _phonon_doc, _band_data, _dos_data), broken markdown cells, and ruff noqa - Add marimo check --strict pre-commit hook - Fix RUF005 in bar_plot.py, TRY300 in matterviz.py * Update widget demos with realistic fixture data and clearer narratives Replace synthetic band/DOS demo payloads with phonon test fixtures and add concise explanatory headings across notebook, marimo, and VSCode interactive demos. * Guard against marimo VS Code extension kernel crash - Skip js() virtual file call when virtual_files_supported=False to prevent ~14 MB base64 data URL allocations that crash the kernel - Fall back to relative ./@file/ URL when request context is unavailable - Set resolved ESM URL on class (not per-instance) to avoid N copies - Use display() from IPython for plot widgets that shadow the method - Document VS Code extension limitation in MatterVizWidget docstring * Simplify marimo ESM caching in MatterVizWidget Use dedicated class-level marimo URL cache fields instead of overloading the generic asset tuple cache, and keep test cache cleanup aligned with the new cache state. * Auto-discover to_dict fields from synced traitlets and reorganize tests Replace manual state_fields tuples with automatic discovery via self.traits(sync=True) in MatterVizWidget.to_dict(). This fixes serialization for ScatterPlotWidget and HistogramWidget which were missing their state_fields definitions, and eliminates the need for every subclass to maintain a redundant field list. Also simplify production code (DRY numpy conversion in normalize_plot_json, loop-ify lattice params, remove narrating comments in trajectory.py) and split test_new_widgets.py into test_normalize.py, test_widget_construction.py, and test_mime.py organized by domain rather than chronology. * Rename display() to show(), add configure_assets(), fix marimo cell deps - Rename MatterVizWidget.display() to show() to avoid shadowing the 'display' traitlet on ScatterPlot/BarPlot/HistogramWidget which made widget.display() raise TypeError: 'dict' object is not callable - Add configure_assets(version=, esm_src=, css_src=) for global asset override via version tags, HTTP URLs, or local file paths - Fix marimo demo: cells now properly return and declare dependencies for _struct, _band_data, _phonon_doc, _dos_data - Simplify matterviz.py: flatten _marimo_esm_url resolution, trim docstrings, merge isinstance checks in normalize_plot_json * Add CI job to execute widget demos Runs all three widget demos (vscode, marimo, jupyter) in a single consolidated step to catch runtime errors in demo scripts.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ScatterPlotWidget,BarPlotWidget, andHistogramWidgetwith shared plot-series normalization inpymatviz.widgets._normalizeTrajectoryWidgetschema handling for emptyspecieslists and expand trajectory/widget regression testsTest plan
pytest -q tests/widgets/test_new_widgets.py tests/widgets/test_trajectory_widget.pyruff,ty,deno,eslint, etc.)Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores