Add element and site projected phonon DOS support#339
Conversation
- extend phonon_dos() with CompletePhononDos projection modes and optional total overlays - harden DOS normalization with explicit error handling for zero-density and invalid integral inputs - expand phonon plotly coverage with projected DOS behavior and normalization edge-case tests - update phonon fixtures, example scripts, and README links for the new projected DOS workflow
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds element/site projection support to phonon DOS plotting via CompletePhononDos and a new project parameter; centralizes DOS preparation (conversion, smearing, normalization); standardizes JSON reads to text mode and removes atomate2 exit guards; introduces a Plotly show_figure helper and accompanying tests/fixture updates. Changes
Sequence DiagramsequenceDiagram
participant User as User/Script
participant Pho as phonon_dos()
participant Val as Validator
participant Proj as Projection Logic
participant Prep as DOS Preparer
participant Plot as Plotly Renderer
User->>Pho: call phonon_dos(doses=CompletePhononDos, project="element")
Pho->>Val: validate inputs & params
Val-->>Pho: validation OK
Pho->>Proj: decompose CompletePhononDos by element/site
Proj->>Prep: for each partial DOS -> convert units, smear, normalize
Prep-->>Proj: prepared freq & density arrays
Proj->>Pho: assemble per-entity traces (+ optional total)
Pho->>Plot: add traces and call show_figure()
Plot->>User: render/display Plotly figure
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pymatviz/phonons/figures.py`:
- Around line 408-459: The non-projection branch currently rejects
CompletePhononDos even though the API accepts it; modify the standard path that
builds dos_dict so that when project is None you accept CompletePhononDos by
converting it to a PhononDos (use PhononDos(dos.frequencies, dos.densities)),
e.g. add an elif isinstance(dos, CompletePhononDos) branch in the loop over
raw_doses to set dos_dict[key] = PhononDos(dos.frequencies, dos.densities), and
keep the existing TypeError only for truly unsupported types; reference symbols:
CompletePhononDos, PhononDos, dos_dict, raw_doses, project.
- simplify phonon_dos normalization and projection control flow - support CompletePhononDos total plotting when project is None - add regression test and refresh README source links from hooks
- streamline phonon_dos projection handling with one validated branch - reduce repetition in phonon DOS example plotting via looped scenarios - preserve behavior while keeping tests and type checks green
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@assets/scripts/phonons/phonon_dos.py`:
- Around line 57-58: The linter flags long error messages in the raise
statements checking phonopy_nacl.total_dos; to satisfy TRY003 either suppress
the rule or use a custom exception—quick fix: append "# noqa: TRY003" to the two
raise statements that reference phonopy_nacl.total_dos (the one checking if
phonopy_nacl.total_dos is None and the similar block around lines 73-76) so the
linter ignores the long message, or alternatively define a short-named custom
exception (e.g., PhonopyDOSError) and raise that from those checks instead of
RuntimeError.
🧹 Nitpick comments (1)
pymatviz/phonons/figures.py (1)
467-491: Inconsistent array mutation pattern.Line 474 uses in-place division (
densities /= density_norm) while line 491 creates a new array (densities = densities / ...). While this should work correctly sinceget_smeared_densitiestypically returns a new array, using the same pattern throughout would be clearer and safer against potential future changes to the upstream method.♻️ Suggested change for consistency
if normalize in ("max", "sum"): density_norm = densities.max() if normalize == "max" else densities.sum() if density_norm == 0: msg_key = "max density" if normalize == "max" else "sum density" raise ValueError( f"Cannot normalize DOS with mode={normalize!r}: {msg_key} is 0." ) - densities /= density_norm + densities = densities / density_norm
- Flatten nested ternary into single conditional - Use np.zeros_like fallback instead of None check
Closes #222
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation
Tests