Add widget state export helpers and harden projected DOS tests#340
Merged
Add widget state export helpers and harden projected DOS tests#340
Conversation
- add `display()` convenience method to `MatterVizWidget` for notebook usage - add `to_dict()` to export stable public widget state fields - add parameterized tests to verify exported keys and runtime state updates
janosh
added
phonons
|
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:
📝 WalkthroughWalkthroughBumps pre-commit hook versions; adds two specific Phonopy DOS-missing exceptions; updates phonon DOS plotting to track per-trace stack groups and adjust stacking fill behavior; exposes MatterVizWidget public state via Changes
Sequence Diagram(s)(omitted — changes do not introduce multi-component sequential control flow warranting a diagram) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
examples/ward_metallic_glasses/formula_features.py (1)
122-134:⚠️ Potential issue | 🟡 MinorFix docstring default for
on_key_err.The docstring still says “Defaults to 'raise'” while the function default is
"set-none"(Line 113). This is misleading for users.🔧 Suggested docstring fix
- Defaults to "raise". + Defaults to "set-none".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/ward_metallic_glasses/formula_features.py` around lines 122 - 134, The docstring for the parameter on_key_err is incorrect: update the default description to match the function signature (change "Defaults to 'raise'" to "Defaults to 'set-none'") in the docstring in examples/ward_metallic_glasses/formula_features.py so the documented default matches the actual default value of the on_key_err parameter (refer to the on_key_err parameter in the function signature).pymatviz/phonons/figures.py (1)
517-523:⚠️ Potential issue | 🟠 MajorReset fill baseline when a new stack group starts.
When
stack=Trueandprojectis set, traces are grouped by their label (e.g., "Label1", "Label2"). The current code always usesfill="tonexty", which fills between the current trace and the previous trace in the figure. This causes misalignment when a new group starts: the first trace of group 2 fills to the last trace of group 1 instead of to the y=0 baseline. The first trace of each group should usefill="tozeroy", with subsequent traces in that group usingfill="tonexty".🔧 Suggested fix
if stack: stack_group = _stack_group(dos_name) - densities = densities + cumulative_density_by_group.get( - stack_group, np.zeros_like(densities) - ) - cumulative_density_by_group[stack_group] = densities - scatter_kwargs["fill"] = "tonexty" + prev = cumulative_density_by_group.get(stack_group) + base = prev if prev is not None else np.zeros_like(densities) + densities = densities + base + cumulative_density_by_group[stack_group] = densities + scatter_kwargs["fill"] = "tonexty" if prev is not None else "tozeroy"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pymatviz/phonons/figures.py` around lines 517 - 523, The current code always sets scatter_kwargs["fill"] = "tonexty", causing the first trace of a new stack group to fill to the previous group's last trace; update the logic around stack_group/_stack_group and cumulative_density_by_group so that when a stack_group is seen for the first time (i.e., stack_group not in cumulative_density_by_group) you set scatter_kwargs["fill"] = "tozeroy" and initialize cumulative_density_by_group[stack_group] to an appropriate zero baseline (e.g., np.zeros_like(densities)) before adding densities, otherwise for subsequent traces keep scatter_kwargs["fill"] = "tonexty". Ensure this check occurs where densities are combined and before fig.add_scatter is called.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/ward_metallic_glasses/formula_features.py`:
- Around line 338-345: The print loop assumes numeric feature values and uses
f"{...:.2f}", which will TypeError when calc_liu_features returns
Optional[float]; update the printing for both the Pt50P50 and test_comp2 loops
to guard each value (e.g., check if features[feature].get(test_comp) is None)
and print a safe fallback like "N/A" (or format the float with :.2f only when
not None) so that missing binary systems don't crash the print routine;
references: calc_liu_features, features dict, test_comp and test_comp2 loops.
---
Outside diff comments:
In `@examples/ward_metallic_glasses/formula_features.py`:
- Around line 122-134: The docstring for the parameter on_key_err is incorrect:
update the default description to match the function signature (change "Defaults
to 'raise'" to "Defaults to 'set-none'") in the docstring in
examples/ward_metallic_glasses/formula_features.py so the documented default
matches the actual default value of the on_key_err parameter (refer to the
on_key_err parameter in the function signature).
In `@pymatviz/phonons/figures.py`:
- Around line 517-523: The current code always sets scatter_kwargs["fill"] =
"tonexty", causing the first trace of a new stack group to fill to the previous
group's last trace; update the logic around stack_group/_stack_group and
cumulative_density_by_group so that when a stack_group is seen for the first
time (i.e., stack_group not in cumulative_density_by_group) you set
scatter_kwargs["fill"] = "tozeroy" and initialize
cumulative_density_by_group[stack_group] to an appropriate zero baseline (e.g.,
np.zeros_like(densities)) before adding densities, otherwise for subsequent
traces keep scatter_kwargs["fill"] = "tonexty". Ensure this check occurs where
densities are combined and before fig.add_scatter is called.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
assets/scripts/cluster/composition/cluster_compositions_matbench.pyexamples/ward_metallic_glasses/formula_features.pypymatviz/brillouin.pypymatviz/classify/confusion_matrix.pypymatviz/cluster/composition/plot.pypymatviz/phonons/figures.pypymatviz/powerups.pypymatviz/rdf/figures.pypymatviz/structure/figures.pypymatviz/structure/helpers.pyreadme.mdtests/test_brillouin.pytests/widgets/test_new_widgets.py
✅ Files skipped from review due to trivial changes (4)
- pymatviz/brillouin.py
- pymatviz/structure/figures.py
- tests/test_brillouin.py
- pymatviz/rdf/figures.py
🚧 Files skipped from review as they are similar to previous changes (1)
- readme.md
- add strict XRD normalization support for canonical and Ferrox dict schemas with validation and hkls normalization - enforce exactly one Fermi surface data source and strengthen widget tests by asserting meaningful normalized state - fix projected DOS stacked fill baseline per model group and update metallic-glass example docs/output formatting for optional values
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/phonons/test_phonon_plotly.py (1)
402-414: Minor redundancy: same trace lookup performed twice.
first_group_trace(lines 409-413) performs the identical lookup as lines 403-407, finding the same trace. Consider reusing the result:♻️ Suggested simplification
for model_label in model_labels: - trace_name = next( - trace.name + first_group_trace = next( + trace for trace in fig_stacked.data if trace.name.startswith(f"{model_label} - ") ) - assert np.allclose(stacked_by_name[trace_name], unstacked_by_name[trace_name]) - first_group_trace = next( - trace - for trace in fig_stacked.data - if trace.name.startswith(f"{model_label} - ") - ) + assert np.allclose( + stacked_by_name[first_group_trace.name], + unstacked_by_name[first_group_trace.name], + ) assert first_group_trace.fill == "tozeroy"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phonons/test_phonon_plotly.py` around lines 402 - 414, The test redundantly looks up the same trace twice; compute the trace once by replacing the first generator that finds trace_name/trace with a single variable (e.g., found_trace = next(trace for trace in fig_stacked.data if trace.name.startswith(f"{model_label} - "))), use found_trace.name for the stacked_by_name/unstacked_by_name comparison and then assert found_trace.fill == "tozeroy", and remove the duplicate generator expressions that create trace_name/first_group_trace separately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/phonons/test_phonon_plotly.py`:
- Around line 402-414: The test redundantly looks up the same trace twice;
compute the trace once by replacing the first generator that finds
trace_name/trace with a single variable (e.g., found_trace = next(trace for
trace in fig_stacked.data if trace.name.startswith(f"{model_label} - "))), use
found_trace.name for the stacked_by_name/unstacked_by_name comparison and then
assert found_trace.fill == "tozeroy", and remove the duplicate generator
expressions that create trace_name/first_group_trace separately.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
examples/ward_metallic_glasses/formula_features.pypymatviz/phonons/figures.pypymatviz/widgets/_normalize.pypymatviz/widgets/fermi_surface.pypymatviz/widgets/xrd.pyreadme.mdtests/phonons/test_phonon_plotly.pytests/widgets/test_matterviz.pytests/widgets/test_new_widgets.py
✅ Files skipped from review due to trivial changes (1)
- pymatviz/widgets/xrd.py
🚧 Files skipped from review as they are similar to previous changes (2)
- readme.md
- tests/widgets/test_matterviz.py
- detect canonical and Ferrox schemas only when all required keys are present, and keep targeted errors for partial schemas - add regression coverage for mixed-key XRD dicts that should resolve through complete Ferrox schema - simplify projected DOS stack reset test by reusing one located trace per model label
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pymatviz/widgets/_normalize.py (1)
133-156: hkls normalization logic is correct but could benefit from helper extraction.The nested conditional checks for different hkls formats work correctly, but the deep nesting and repeated structure validation make this harder to maintain. Consider extracting a helper function like
_normalize_ferrox_hkls(hkls_data)if this grows more complex.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pymatviz/widgets/_normalize.py` around lines 133 - 156, Extract the nested hkls validation/normalization logic into a helper function (e.g., _normalize_ferrox_hkls(hkls_data)) and replace the current inlined checks inside the block that reads hkls_data = obj.get("hkls") with a call to that helper; the helper should accept hkls_data, perform the two validation branches (single hkl entries as [h,k,l] and grouped entries where first element is [h,k,l]) and return the normalized list or the original hkls_data fallback, so update the assignment to normalized["hkls"] = _normalize_ferrox_hkls(hkls_data).
🤖 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/_normalize.py`:
- Around line 133-156: Extract the nested hkls validation/normalization logic
into a helper function (e.g., _normalize_ferrox_hkls(hkls_data)) and replace the
current inlined checks inside the block that reads hkls_data = obj.get("hkls")
with a call to that helper; the helper should accept hkls_data, perform the two
validation branches (single hkl entries as [h,k,l] and grouped entries where
first element is [h,k,l]) and return the normalized list or the original
hkls_data fallback, so update the assignment to normalized["hkls"] =
_normalize_ferrox_hkls(hkls_data).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pymatviz/widgets/_normalize.pytests/phonons/test_phonon_plotly.pytests/widgets/test_new_widgets.py
- extract Ferrox HKL normalization into a helper for cleaner XRD input handling - validate trajectory dict schemas with clearer type/key errors while preserving legacy structure-frame dicts - add and refine trajectory widget regression tests for invalid schemas and restored lifecycle state
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/widgets/test_trajectory_widget.py (1)
423-496: Comprehensive schema validation test coverage.The parameterized test cases effectively exercise the validation error paths in
_validate_trajectory_dict. The test at lines 466-488 correctly expects the "coordinate key" error since the validation checksspeciesbeforeabc/xyz.Consider adding test cases for the remaining validation branches if full coverage is desired:
- Missing
labelkey- Missing
propertiesdict- Missing lattice derived keys (
a,b,c,alpha,beta,gamma)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/widgets/test_trajectory_widget.py` around lines 423 - 496, The test suite misses cases exercising some branches in _validate_trajectory_dict; add parameterized cases to test_trajectory_widget_invalid_dict_schema_raises_helpful_error that trigger missing 'label' in a site, missing 'properties' dict on a frame/trajectory, and missing lattice-derived keys ('a','b','c','alpha','beta','gamma') so the TrajectoryWidget(trajectory=...) validation raises the expected ValueError/TypeError with the correct messages; reference the existing test function name test_trajectory_widget_invalid_dict_schema_raises_helpful_error and the validator _validate_trajectory_dict to locate where to extend the parameter list and expected error messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pymatviz/widgets/trajectory.py`:
- Around line 255-259: Remove the duplicate local is_structure_like function and
import the canonical implementation from process_data (from
pymatviz.process_data import is_structure_like); replace usages of the local
function with the imported one so the code relies on the global isinstance
checks against STRUCTURE_CLASSES (which will properly recognize PhonopyAtoms and
other supported types) instead of the weaker attribute-based duck-typing.
---
Nitpick comments:
In `@tests/widgets/test_trajectory_widget.py`:
- Around line 423-496: The test suite misses cases exercising some branches in
_validate_trajectory_dict; add parameterized cases to
test_trajectory_widget_invalid_dict_schema_raises_helpful_error that trigger
missing 'label' in a site, missing 'properties' dict on a frame/trajectory, and
missing lattice-derived keys ('a','b','c','alpha','beta','gamma') so the
TrajectoryWidget(trajectory=...) validation raises the expected
ValueError/TypeError with the correct messages; reference the existing test
function name test_trajectory_widget_invalid_dict_schema_raises_helpful_error
and the validator _validate_trajectory_dict to locate where to extend the
parameter list and expected error messages.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pymatviz/widgets/_normalize.pypymatviz/widgets/trajectory.pytests/widgets/test_trajectory_widget.py
- validate and complete trajectory dict inputs more robustly, including per-site coordinate checks and safer species handling - simplify widget tests by removing tautological/perf-brittle assertions and replacing them with stronger behavioral checks
janosh
added a commit
that referenced
this pull request
Feb 25, 2026
* Add widget display and state export helpers - add `display()` convenience method to `MatterVizWidget` for notebook usage - add `to_dict()` to export stable public widget state fields - add parameterized tests to verify exported keys and runtime state updates * fix ty errors * Improve widget normalization and projected DOS stacking behavior - add strict XRD normalization support for canonical and Ferrox dict schemas with validation and hkls normalization - enforce exactly one Fermi surface data source and strengthen widget tests by asserting meaningful normalized state - fix projected DOS stacked fill baseline per model group and update metallic-glass example docs/output formatting for optional values * Refine XRD schema precedence and test readability - detect canonical and Ferrox schemas only when all required keys are present, and keep targeted errors for partial schemas - add regression coverage for mixed-key XRD dicts that should resolve through complete Ferrox schema - simplify projected DOS stack reset test by reusing one located trace per model label * Harden trajectory dict handling and normalize Ferrox HKLs - extract Ferrox HKL normalization into a helper for cleaner XRD input handling - validate trajectory dict schemas with clearer type/key errors while preserving legacy structure-frame dicts - add and refine trajectory widget regression tests for invalid schemas and restored lifecycle state * Harden trajectory normalization tests - validate and complete trajectory dict inputs more robustly, including per-site coordinate checks and safer species handling - simplify widget tests by removing tautological/perf-brittle assertions and replacing them with stronger behavioral checks * fix ty
janosh
added a commit
that referenced
this pull request
Feb 25, 2026
* Add widget display and state export helpers - add `display()` convenience method to `MatterVizWidget` for notebook usage - add `to_dict()` to export stable public widget state fields - add parameterized tests to verify exported keys and runtime state updates * fix ty errors * Improve widget normalization and projected DOS stacking behavior - add strict XRD normalization support for canonical and Ferrox dict schemas with validation and hkls normalization - enforce exactly one Fermi surface data source and strengthen widget tests by asserting meaningful normalized state - fix projected DOS stacked fill baseline per model group and update metallic-glass example docs/output formatting for optional values * Refine XRD schema precedence and test readability - detect canonical and Ferrox schemas only when all required keys are present, and keep targeted errors for partial schemas - add regression coverage for mixed-key XRD dicts that should resolve through complete Ferrox schema - simplify projected DOS stack reset test by reusing one located trace per model label * Harden trajectory dict handling and normalize Ferrox HKLs - extract Ferrox HKL normalization into a helper for cleaner XRD input handling - validate trajectory dict schemas with clearer type/key errors while preserving legacy structure-frame dicts - add and refine trajectory widget regression tests for invalid schemas and restored lifecycle state * Harden trajectory normalization tests - validate and complete trajectory dict inputs more robustly, including per-site coordinate checks and safer species handling - simplify widget tests by removing tautological/perf-brittle assertions and replacing them with stronger behavioral checks * fix ty + update readme citations
2 tasks
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
MatterVizWidget.display()andMatterVizWidget.to_dict()with an explicit publicstate_fieldscontractTest plan
pytest tests/widgets/test_matterviz.py tests/phonons/test_phonon_plotly.py -qto_dict()key export and projected DOS stacking/total-overlay behaviorruff,ty, docs/link checks, formatting/lint hooks)Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Tests
Chores