fix(py): let local_density_of_states choose its broadening - #8
Open
reneotten wants to merge 1 commit into
Open
Conversation
`local_density_of_states()` always passed `negf::DEFAULT_ETA` (1e-8 eV) to `green_function_sweep`, with no way to override it from Python. That default is ~5 orders of magnitude narrower than the default 1 meV energy grid (`DeviceParams::d_e`), so resonances land between grid points: sampling a 719 x 561 map at the default device leaves only ~0.3% of entries non-negligible. Plotted on a log scale the result looks empty, and the failure is silent -- no error, just a blank figure. It also means that after `solve_self_consistent()` (which defaults to `eta = 0.08`) the LDOS could not be evaluated at the same broadening the loop itself used. Add optional `eta` and `d_e` arguments. Defaults are unchanged, so existing no-argument callers keep the original MATLAB sharp-resonance behaviour. - eta: broadening in eV; rejects non-finite / non-positive values - d_e: per-call energy grid override in eV; defaults to DeviceParams::d_e - document the eta-vs-d_e pitfall and the (nE,) / (nE, n) return shapes Adds a regression test asserting that a larger eta resolves more of the spectrum on a fixed grid (verified to fail if eta is not actually plumbed through).
There was a problem hiding this comment.
Pull request overview
This PR updates the Python-facing LDOS API so callers can control NEGF broadening (eta) and (optionally) the energy grid spacing (d_e), addressing the “blank LDOS plot” failure mode caused by using an extremely narrow default broadening on a comparatively coarse default energy grid.
Changes:
- Add optional
etaandd_eparameters toDevice.local_density_of_states()in the Python wrapper and plumb them into the PyO3 binding. - Validate
etaandd_ein the Rust/PyO3 layer (finite and strictly positive) and keep defaults consistent with existing behavior. - Add a core regression test asserting that larger
etayields a more “filled” spectrum on a fixed grid.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| python/negforge/init.py | Exposes eta/d_e in the Python wrapper and documents return shapes + the eta vs d_e pitfall. |
| crates/negforge-py/src/lib.rs | Adds PyO3 signature for eta/d_e, validates inputs, and forwards eta into green_function_sweep. |
| crates/negforge-core/src/negf.rs | Adds a regression test asserting increased broadening resolves more of the LDOS on a fixed grid. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+421
to
+424
| assert!( | ||
| broadened > 0.5, | ||
| "eta a few times the grid step should fill most of the map, got {broadened:.4}" | ||
| ); |
| return self._inner.screening_length | ||
|
|
||
| def local_density_of_states(self): | ||
| def local_density_of_states(self, eta: float | None = None, d_e: float | None = None): |
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
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.
fix(py): let local_density_of_states choose its broadening
always passed (1e-8 eV) to
, with no way to override it from Python.
That default is ~5 orders of magnitude narrower than the default 1 meV
energy grid (), so resonances land between grid points:
sampling a 719 x 561 map at the default device leaves only ~0.3% of entries
non-negligible. Plotted on a log scale the result looks empty, and the
failure is silent -- no error, just a blank figure. It also means that after
(which defaults to ) the LDOS could not
be evaluated at the same broadening the loop itself used.
Add optional and arguments. Defaults are unchanged, so existing
no-argument callers keep the original MATLAB sharp-resonance behaviour.
Adds a regression test asserting that a larger eta resolves more of the
spectrum on a fixed grid (verified to fail if eta is not actually plumbed
through).