add argument for TBSystem to change eig_solver#325
Open
Lonya0 wants to merge 5 commits intodeepmodeling:mainfrom
Open
add argument for TBSystem to change eig_solver#325Lonya0 wants to merge 5 commits intodeepmodeling:mainfrom
Lonya0 wants to merge 5 commits intodeepmodeling:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds flexible keyword-argument propagation for eigenvalue/overlap configuration across postprocessing: Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant BandAccessor
participant TBSystem
participant Calculator
participant EigSolver
Client->>BandAccessor: compute(**kwargs with nk, eig_solver)
BandAccessor->>TBSystem: request bands / get_efermi(**kwargs)
TBSystem->>Calculator: get_eigenvalues(atomic_data, **kwargs)
Calculator->>EigSolver: eigv_solver(data, nk=resolved_nk, eig_solver=resolved_solver)
EigSolver-->>Calculator: eigenvalues
Calculator-->>TBSystem: eigenvalues, metadata
TBSystem-->>BandAccessor: bands / efermi
BandAccessor-->>Client: bands (computed)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 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)
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}
Contributor
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 (3)
dptb/postprocess/elec_struc_cal.py (1)
107-123:⚠️ Potential issue | 🔴 CriticalPreserve
Falseas a real opt-out for overlap overrides.Line 154 converts
override_overlap=FalseintoNone, and Lines 107-123 then fall back toself.override_overlapagain. On non-overlap models that also leavesself.eigvconfigured with overlap fields after the first overridden call, so a later opt-out can still use the default overlap file or fail because no overlap data was loaded. Please normalize once per call (None=> instance default,False=> disable,str=> explicit path) and derive both theself.eigvconfiguration andload_data_for_model(..., override_overlap=...)from that normalized value.Also applies to: 154-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dptb/postprocess/elec_struc_cal.py` around lines 107 - 123, Normalize the incoming override_overlap once at the start of the call: treat None as "use instance default" by setting normalized = self.override_overlap if override_overlap is None else override_overlap; preserve False as a real opt-out and keep strings/paths as explicit values. Use this normalized value to decide whether to instantiate self.eigv (i.e., only create Eigenvalues when normalized is not False and indicates overlap) and pass the same normalized value into load_data_for_model(..., override_overlap=normalized) so you never convert False to None or re-fallback to self.override_overlap later; update uses of override_overlap/self.override_overlap and the Eigenvalues construction to reference this one normalized variable.dptb/postprocess/unified/system.py (2)
299-312:⚠️ Potential issue | 🟠 Major
reuse=Truemakes the new band options inert after the first computation.Once
self.has_bandsis set, a laterget_bands(..., eig_solver="numpy")or differentnkstill returns the cached accessor from the earlier run because the reuse check ignores the new compute kwargs. The new API should either invalidate the cache when these options change or include them in the cache key.🗂️ Track the band-compute options in the reuse check
- if self.has_bands and reuse: + requested_band_kwargs = { + "nk": kwargs.get("nk", None), + "eig_solver": kwargs.get("eig_solver", None), + } + if self.has_bands and reuse and getattr(self, "_last_band_kwargs", None) == requested_band_kwargs: return self._bands else: assert kpath_config is not None, "kpath_config must be provided if bands not calculated." self._bands = BandAccessor(self) self._bands.set_kpath(**kpath_config) - self._bands.compute(nk=kwargs.get("nk", None), - eig_solver=kwargs.get("eig_solver", None)) + self._bands.compute(**requested_band_kwargs) + self._last_band_kwargs = requested_band_kwargs self.has_bands = True return self._bands🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dptb/postprocess/unified/system.py` around lines 299 - 312, get_bands currently returns a cached BandAccessor (self._bands) whenever self.has_bands is true regardless of new compute options (nk, eig_solver), causing new options to be ignored; modify get_bands to detect when provided compute kwargs differ from the options used to produce the cached accessor and either invalidate/recreate the cache or re-run compute on the cached BandAccessor. Concretely: record the last compute options on the BandAccessor (e.g., BandAccessor.last_compute_options) when compute(...) is called, then in get_bands compare kwargs (nk, eig_solver and any other relevant keys) against that record and if they differ and reuse is True either call self._bands.compute(...) with the new options or set reuse=False/replace self._bands so the new options take effect; ensure references to get_bands, self._bands, BandAccessor.compute, and self.has_bands are updated accordingly.
243-251:⚠️ Potential issue | 🔴 CriticalDon't pass calculator-only kwargs into
estimate_efermi_e().After the
get_eigenvaluescall,kwargsstill containsnk/eig_solver, and Line 251 forwards them intoestimate_efermi_e(), which accepts neither name.TBSystem.get_efermi(..., eig_solver="numpy")will raiseTypeErrorinstead of using the new feature.🧩 Split solver kwargs from Fermi-level kwargs
- data, eigs = self.calculator.get_eigenvalues(data, - nk=kwargs.get("nk", None), - eig_solver=kwargs.get("eig_solver", None)) + solver_kwargs = {} + fermi_kwargs = dict(kwargs) + for key in ("nk", "eig_solver"): + value = fermi_kwargs.pop(key, None) + if value is not None: + solver_kwargs[key] = value + + data, eigs = self.calculator.get_eigenvalues(data, **solver_kwargs) calculated_efermi = self.estimate_efermi_e( eigenvalues=eigs.detach().numpy(), temperature = temperature, smearing_method=smearing_method, - q_tol = q_tol, **kwargs) + q_tol = q_tol, **fermi_kwargs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dptb/postprocess/unified/system.py` around lines 243 - 251, The call to estimate_efermi_e is accidentally receiving calculator-only kwargs (e.g., "nk" and "eig_solver") that were meant for get_eigenvalues; update the code around the get_eigenvalues / estimate_efermi_e sequence to separate solver kwargs from Fermi-level kwargs — e.g., extract or copy kwargs, pop or exclude "nk" and "eig_solver" before calling estimate_efermi_e (or build an explicit fermi_kwargs dict with only the allowed keys), leaving get_eigenvalues(...) using kwargs.get("nk"/"eig_solver") and calling estimate_efermi_e(eigenvalues=eigs.detach().numpy(), temperature=temperature, smearing_method=smearing_method, q_tol=q_tol, **fermi_kwargs) so no calculator-only names are forwarded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dptb/postprocess/unified/properties/band.py`:
- Around line 235-237: The call to self._system.calculator.get_eigenvalues
currently always forwards nk and eig_solver with None which breaks compatibility
with calculators implementing the old get_eigenvalues(data) signature; instead,
build a small kwargs dict (e.g., eig_kwargs) conditionally: add 'nk' only if
'nk' in kwargs and add 'eig_solver' only if 'eig_solver' in kwargs, then call
self._system.calculator.get_eigenvalues(data, **eig_kwargs). This preserves
compatibility with TBSystem calculators while still forwarding explicitly
provided arguments.
---
Outside diff comments:
In `@dptb/postprocess/elec_struc_cal.py`:
- Around line 107-123: Normalize the incoming override_overlap once at the start
of the call: treat None as "use instance default" by setting normalized =
self.override_overlap if override_overlap is None else override_overlap;
preserve False as a real opt-out and keep strings/paths as explicit values. Use
this normalized value to decide whether to instantiate self.eigv (i.e., only
create Eigenvalues when normalized is not False and indicates overlap) and pass
the same normalized value into load_data_for_model(...,
override_overlap=normalized) so you never convert False to None or re-fallback
to self.override_overlap later; update uses of
override_overlap/self.override_overlap and the Eigenvalues construction to
reference this one normalized variable.
In `@dptb/postprocess/unified/system.py`:
- Around line 299-312: get_bands currently returns a cached BandAccessor
(self._bands) whenever self.has_bands is true regardless of new compute options
(nk, eig_solver), causing new options to be ignored; modify get_bands to detect
when provided compute kwargs differ from the options used to produce the cached
accessor and either invalidate/recreate the cache or re-run compute on the
cached BandAccessor. Concretely: record the last compute options on the
BandAccessor (e.g., BandAccessor.last_compute_options) when compute(...) is
called, then in get_bands compare kwargs (nk, eig_solver and any other relevant
keys) against that record and if they differ and reuse is True either call
self._bands.compute(...) with the new options or set reuse=False/replace
self._bands so the new options take effect; ensure references to get_bands,
self._bands, BandAccessor.compute, and self.has_bands are updated accordingly.
- Around line 243-251: The call to estimate_efermi_e is accidentally receiving
calculator-only kwargs (e.g., "nk" and "eig_solver") that were meant for
get_eigenvalues; update the code around the get_eigenvalues / estimate_efermi_e
sequence to separate solver kwargs from Fermi-level kwargs — e.g., extract or
copy kwargs, pop or exclude "nk" and "eig_solver" before calling
estimate_efermi_e (or build an explicit fermi_kwargs dict with only the allowed
keys), leaving get_eigenvalues(...) using kwargs.get("nk"/"eig_solver") and
calling estimate_efermi_e(eigenvalues=eigs.detach().numpy(),
temperature=temperature, smearing_method=smearing_method, q_tol=q_tol,
**fermi_kwargs) so no calculator-only names are forwarded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a5b75510-6833-4147-bef2-9a7abaed6897
📒 Files selected for processing (4)
dptb/postprocess/elec_struc_cal.pydptb/postprocess/unified/calculator.pydptb/postprocess/unified/properties/band.pydptb/postprocess/unified/system.py
Comment on lines
+235
to
+237
| data, eigs = self._system.calculator.get_eigenvalues(data, | ||
| nk=kwargs.get("nk", None), | ||
| eig_solver=kwargs.get("eig_solver", None)) |
Contributor
There was a problem hiding this comment.
Only forward solver kwargs when they were explicitly requested.
TBSystem still allows duck-typed calculators, so always calling get_eigenvalues(..., nk=None, eig_solver=None) is a compatibility regression for calculators that still implement the old get_eigenvalues(data) signature. Build the kwargs dict conditionally before the call.
🔧 Safer forwarding
- data, eigs = self._system.calculator.get_eigenvalues(data,
- nk=kwargs.get("nk", None),
- eig_solver=kwargs.get("eig_solver", None))
+ calc_kwargs = {}
+ if kwargs.get("nk") is not None:
+ calc_kwargs["nk"] = kwargs["nk"]
+ if kwargs.get("eig_solver") is not None:
+ calc_kwargs["eig_solver"] = kwargs["eig_solver"]
+
+ data, eigs = self._system.calculator.get_eigenvalues(data, **calc_kwargs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dptb/postprocess/unified/properties/band.py` around lines 235 - 237, The call
to self._system.calculator.get_eigenvalues currently always forwards nk and
eig_solver with None which breaks compatibility with calculators implementing
the old get_eigenvalues(data) signature; instead, build a small kwargs dict
(e.g., eig_kwargs) conditionally: add 'nk' only if 'nk' in kwargs and add
'eig_solver' only if 'eig_solver' in kwargs, then call
self._system.calculator.get_eigenvalues(data, **eig_kwargs). This preserves
compatibility with TBSystem calculators while still forwarding explicitly
provided arguments.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dptb/postprocess/unified/system.py (1)
300-303: Consider forwarding**kwargsdirectly inget_bandsfor extensibility.
get_bandsnow accepts**kwargs, but Line 312 forwards onlynk/eig_solver. Passing through**kwargskeeps this method aligned with the unified flexible-kwargs direction and avoids future key plumbing edits.Refactor suggestion
- self._bands.compute(nk=kwargs.get("nk", None), - eig_solver=kwargs.get("eig_solver", None)) + self._bands.compute(**kwargs)Also applies to: 312-313
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dptb/postprocess/unified/system.py` around lines 300 - 303, The get_bands function currently accepts **kwargs but only forwards specific keys (nk/eig_solver) when calling the downstream method; change the call inside get_bands (the block that currently passes nk and eig_solver) to forward **kwargs as well (or instead) so all additional parameters are propagated; update the invocation in get_bands to include **kwargs when calling the underlying routine (referencing get_bands) to preserve extensibility and avoid future plumbing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dptb/postprocess/unified/system.py`:
- Around line 243-245: The call to self.calculator.get_eigenvalues currently
injects nk and eig_solver unconditionally (using kwargs.get("nk", None) and
kwargs.get("eig_solver", None)), which can cause TypeError for calculator
implementations that don't accept those keywords; change the call to build a
filtered kwargs dict (e.g., include "nk" and "eig_solver" only if kwargs
contains non-None values) and pass that dict (or explicitly pass only the
provided parameters) to self.calculator.get_eigenvalues so you only supply
supported keywords.
---
Nitpick comments:
In `@dptb/postprocess/unified/system.py`:
- Around line 300-303: The get_bands function currently accepts **kwargs but
only forwards specific keys (nk/eig_solver) when calling the downstream method;
change the call inside get_bands (the block that currently passes nk and
eig_solver) to forward **kwargs as well (or instead) so all additional
parameters are propagated; update the invocation in get_bands to include
**kwargs when calling the underlying routine (referencing get_bands) to preserve
extensibility and avoid future plumbing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1982fbdd-2a60-49e0-a62a-672ae3d165a4
📒 Files selected for processing (1)
dptb/postprocess/unified/system.py
Comment on lines
+243
to
+245
| data, eigs = self.calculator.get_eigenvalues(data, | ||
| nk=kwargs.get("nk", None), | ||
| eig_solver=kwargs.get("eig_solver", None)) |
Contributor
There was a problem hiding this comment.
Avoid unconditional keyword injection into protocol calculators.
At Line 243, get_eigenvalues is called with nk= and eig_solver= unconditionally (even when both are None). Since Line 46 allows any calculator that merely has get_eigenvalues, this can raise TypeError for valid protocol implementations that don’t accept those keywords.
Proposed compatibility-safe patch
- data, eigs = self.calculator.get_eigenvalues(data,
- nk=kwargs.get("nk", None),
- eig_solver=kwargs.get("eig_solver", None))
+ eigen_kwargs = {}
+ nk = kwargs.get("nk", None)
+ eig_solver = kwargs.get("eig_solver", kwargs.get("solver", None))
+ if nk is not None:
+ eigen_kwargs["nk"] = nk
+ if eig_solver is not None:
+ eigen_kwargs["eig_solver"] = eig_solver
+ data, eigs = self.calculator.get_eigenvalues(data, **eigen_kwargs)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| data, eigs = self.calculator.get_eigenvalues(data, | |
| nk=kwargs.get("nk", None), | |
| eig_solver=kwargs.get("eig_solver", None)) | |
| eigen_kwargs = {} | |
| nk = kwargs.get("nk", None) | |
| eig_solver = kwargs.get("eig_solver", kwargs.get("solver", None)) | |
| if nk is not None: | |
| eigen_kwargs["nk"] = nk | |
| if eig_solver is not None: | |
| eigen_kwargs["eig_solver"] = eig_solver | |
| data, eigs = self.calculator.get_eigenvalues(data, **eigen_kwargs) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dptb/postprocess/unified/system.py` around lines 243 - 245, The call to
self.calculator.get_eigenvalues currently injects nk and eig_solver
unconditionally (using kwargs.get("nk", None) and kwargs.get("eig_solver",
None)), which can cause TypeError for calculator implementations that don't
accept those keywords; change the call to build a filtered kwargs dict (e.g.,
include "nk" and "eig_solver" only if kwargs contains non-None values) and pass
that dict (or explicitly pass only the provided parameters) to
self.calculator.get_eigenvalues so you only supply supported keywords.
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.
#324
and change override_overlap argument passing
Summary by CodeRabbit