Feat: Add new unifined postprocess class to handle DeePTB model. #301
Conversation
Enhance build_model to handle 'poly2' and 'poly4' as special checkpoint names, automatically resolving them to their corresponding base model files in the dftb directory. This simplifies model initialization for common polynomial baselines.
Add is_gui_available() function to detect matplotlib GUI display availability in various environments including Jupyter notebooks and different operating systems. Refactor HamiltonianCalculator from Protocol to ABC with proper abstract methods for better type safety and inheritance. Update DeePTBAdapter to inherit from the new abstract base class.
…ncluding new examples and tests.
Add new abstract method get_hk() to calculate H(k) and S(k) matrices at specified k-points. Rename get_hamiltonian_blocks() to get_hr() for clarity. Implement get_hk() in DeePTBAdapter with support for custom k-point injection and proper tensor handling. Remove redundant get_hamiltonian_at_k() method from BandAccessor. Add data property to TBSystem for atomic data access.
- Add abstract get_eigenstates method to HamiltonianCalculator interface - Implement eigenstates calculation in DeePTBAdapter using Eigh solver - Add DosData class for structured DOS results storage - Add DOS plotting functionality with matplotlib integration - Refactor solver initialization to support both Eigenvalues and Eigh solvers - Remove unused logging imports and update error handling
Rename set_dos to set_dos_config for clarity in DOS configuration setup. Add reuse parameter to get_dos and get_bands methods to control recalculation. Improve error messages and add dos_data property for easier access to computed DOS results. Refactor DOS initialization to separate kpoints setup from configuration.
…data Replace mock-based unit tests with integration tests that use actual silicon example data. Add pytest fixtures for efficient system initialization, test band structure and DOS calculations with real model, and include plotting functionality tests. This provides more realistic validation of the unified postprocessing module.
Add a comprehensive Jupyter notebook demonstrating the new TBSystem class for unified post-processing in DeePTB. The tutorial covers initialization, band structure calculation, DOS computation, and visualization using the centralized interface that manages both atomic structure and Hamiltonian model.
QG-phy
changed the title
📝 WalkthroughWalkthroughAdds a unified post-processing layer: TBSystem orchestration, a HamiltonianCalculator interface with DeePTBAdapter, Band/DOS accessors and data classes, Fermi-level utilities and k-point helpers, export utilities, GUI detection, build helpers (checkpoint mapping and option-diff warnings), dtype/precision fixes, tests, and .gitignore updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TBSystem
participant Accessor
participant Calculator
participant Eigensolver
User->>TBSystem: instantiate(data, calculator)
TBSystem->>Calculator: wrap/init model (DeePTBAdapter)
rect rgb(220,240,255)
Note over User,Accessor: Band workflow (lazy)
User->>TBSystem: access .band
TBSystem->>Accessor: instantiate BandAccessor
User->>Accessor: set_kpath(...)
User->>Accessor: compute()
Accessor->>Calculator: model_forward / get_eigenvalues(atomic_data)
Calculator->>Eigensolver: request eigenvalues/eigensystems
Eigensolver-->>Calculator: eigenvalues (± vectors)
Calculator-->>Accessor: eigenvalues (and vectors)
Accessor->>Accessor: build BandStructureData
User->>Accessor: plot/save
end
rect rgb(240,220,255)
Note over User,Accessor: DOS workflow (lazy)
User->>TBSystem: access .dos
TBSystem->>Accessor: instantiate DosAccessor
User->>Accessor: set_kpoints(kmesh)
User->>Accessor: set_dos_config(...)
User->>Accessor: compute()
Accessor->>Calculator: model_forward / get_eigenvalues(atomic_data)
Calculator->>Eigensolver: request eigenvalues
Eigensolver-->>Calculator: eigenvalues
Calculator-->>Accessor: eigenvalues
Accessor->>Accessor: apply broadening -> DosData
User->>Accessor: plot/save
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dptb/nn/build.py (2)
196-197: Fix potential KeyError in non-dict comparison.When
expected_valueis not a dictionary, line 196 directly accessestarget_dict[base_key]without checking if the key exists. This will raise aKeyErrorifbase_keyis missing fromtarget_dict.🔎 Proposed fix
else: - if expected_value != target_dict[base_key]: + if base_key not in target_dict: + log.warning(f"The model option {base_key} is not defined in input model_options, set to {expected_value}.") + elif expected_value != target_dict[base_key]: log.warning(f"The model option {base_key} is set to {expected_value}, but in input it is {target_dict[base_key]}, make sure it it correct!")
197-197: Fix typo in warning message.The warning message contains a typo: "make sure it it correct!" has a duplicate "it".
🔎 Proposed fix
- log.warning(f"The model option {base_key} is set to {expected_value}, but in input it is {target_dict[base_key]}, make sure it it correct!") + log.warning(f"The model option {base_key} is set to {expected_value}, but in input it is {target_dict[base_key]}, make sure it is correct!")
🧹 Nitpick comments (14)
dptb/nn/build.py (3)
187-187: Remove unnecessary deepcopy.The
deepcopyat line 187 is unnecessary because the function only reads fromtarget_dictand never modifies it. This deepcopy is performed on every recursive call, which is wasteful for large configuration dictionaries.🔎 Proposed refactor
- target_dict= copy.deepcopy(model_options) # 防止修改原始字典 + target_dict = model_options
194-194: Simplify redundant f-string formatting.The f-string
f"{subk}"is redundant sincesubkis already a string. This can be simplified to justsubk.🔎 Proposed fix
- deep_dict_difference(f"{subk}", subv, target2) + deep_dict_difference(subk, subv, target2)
190-190: Add type check before dictionary access.Line 190 assumes
target_dict[base_key]is a dictionary when using.get(), but this isn't verified. Iftarget_dict[base_key]exists but is not a dict, this could lead to unexpected behavior.🔎 Suggested enhancement
if isinstance(expected_value, dict): for subk, subv in expected_value.items(): - if subk not in target_dict.get(base_key, {}): + if base_key not in target_dict: + log.warning(f"The model option {base_key} is not defined in input model_options.") + elif not isinstance(target_dict[base_key], dict): + log.warning(f"The model option {base_key} is expected to be a dict but got {type(target_dict[base_key]).__name__}.") + elif subk not in target_dict[base_key]: log.warning(f"The model option {subk} in {base_key} is not defined in input model_options, set to {subv}.") else: target2 = copy.deepcopy(target_dict[base_key]) deep_dict_difference(f"{subk}", subv, target2)dptb/postprocess/common.py (1)
159-160: Use explicit exception type instead of bareexcept.Bare
except:catches all exceptions includingKeyboardInterruptandSystemExit, which is generally undesirable.🔎 Proposed fix
try: matplotlib.use(test_backend, force=True) test_fig = plt.figure() plt.close(test_fig) matplotlib.use(current_backend, force=True) return True - except: + except Exception: continuedptb/tests/test_postprocess_unified.py (2)
4-4: Unused import:shutil.The
shutilmodule is imported but never used in this test file.🔎 Proposed fix
import pytest import os -import shutil import torch
138-138: Prefix unused variable with underscore.
sr_blocksis unpacked but never used. Prefix with_to indicate it's intentionally unused.🔎 Proposed fix
- hr_blocks, sr_blocks = tbsys.calculator.get_hr(tbsys.data) + hr_blocks, _sr_blocks = tbsys.calculator.get_hr(tbsys.data)dptb/postprocess/unified/properties/band.py (1)
146-154: Remove duplicate docstring.The
BandAccessorclass has the same docstring repeated twice.🔎 Proposed fix
class BandAccessor: """ Accessor for Band Structure functionality on a TBSystem. Allows syntax like: system.band.set_kpath(...) """ - """ - Accessor for Band Structure functionality on a TBSystem. - Allows syntax like: system.band.set_kpath(...) - """ def __init__(self, system: 'TBSystem'):dptb/postprocess/unified/properties/dos.py (3)
31-31: Use explicitOptionalfor nullable parameter.PEP 484 prohibits implicit
Optional. Theselected_orbitalsparameter defaults toNonebut its type hint doesn't includeOptional.🔎 Proposed fix
- def plot(self, filename: Optional[str] = 'dos.png', show: Optional[bool] = None, - xlim: Optional[List[float]] = None, plot_pdos: bool = False, selected_orbitals: Union[List[int], List[str]] = None): + def plot(self, filename: Optional[str] = 'dos.png', show: Optional[bool] = None, + xlim: Optional[List[float]] = None, plot_pdos: bool = False, selected_orbitals: Optional[Union[List[int], List[str]]] = None):
207-216: Silent failure in broadening function for unknown method.The
broadeningfunction returns0for unknown methods, which silently produces incorrect results. Whileset_dos_configvalidates the smearing type, a defensive approach would raise an error here.🔎 Proposed fix
if method == 'gaussian': return np.exp(-0.5 * (delta / sigma)**2) / (np.sqrt(2 * np.pi) * sigma) elif method == 'lorentzian': return (1 / np.pi) * (sigma / (delta**2 + sigma**2)) else: - return 0 + raise ValueError(f"Unknown broadening method: {method}")
247-248: Prefix unused variables with underscore.
nkandnbare unpacked but onlynorbis used.🔎 Proposed fix
- nk, norb, nb = weights.shape + _nk, norb, _nb = weights.shapedptb/postprocess/unified/system.py (3)
32-32: Avoid function call in default argument.
torch.device("cpu")is evaluated once at function definition time. UseNoneas default and handle inside the function.🔎 Proposed fix
def __init__( self, data: Union[AtomicData, ase.Atoms, str], calculator: Union[HamiltonianCalculator, torch.nn.Module, str], override_overlap: Optional[str] = None, - device: Optional[Union[str, torch.device]]= torch.device("cpu") + device: Optional[Union[str, torch.device]] = None ): + if device is None: + device = torch.device("cpu") # Initialize Calculator/Model if isinstance(calculator, str):
131-137: Remove redundantoverlap_flagcheck.
overlap_flagis already checked on line 131 as part of theifcondition, so the inner check on line 136 is alwaysTrueand redundant.🔎 Proposed fix
if overlap_flag and isinstance(override_overlap, str): assert os.path.exists(override_overlap), "Overlap file not found." with h5py.File(override_overlap, "r") as overlap_blocks: if len(overlap_blocks) != 1: log.info('Overlap file contains more than one overlap matrix, only first will be used.') - if overlap_flag: - log.warning('override_overlap is enabled while model contains overlap, override_overlap will be used.') + log.warning('override_overlap is enabled while model contains overlap, override_overlap will be used.')
163-175: Remove unused**kwargsor document its purpose.The
**kwargsargument inget_bandsis never used. If it's intended for future expansion, consider documenting this; otherwise, remove it.🔎 Proposed fix (if removing)
- def get_bands(self, kpath_config: Optional[dict] = None, reuse: Optional[bool]=True, **kwargs): + def get_bands(self, kpath_config: Optional[dict] = None, reuse: Optional[bool]=True):dptb/postprocess/unified/calculator.py (1)
236-240: Split multiple statements across separate lines.For readability, each conditional branch should be on its own line.
🔎 Proposed fix
for atomtype, orb_dict in self.model.idp.basis.items(): orb_list = [] for o in orb_dict: - if "s" in o: orb_list.append(o) - elif "p" in o: orb_list.extend([o+"_y", o+"_z", o+"_x"]) # Standard Wannier90 p-order usually z,x,y or similar? keeping dptb order - elif "d" in o: orb_list.extend([o+"_xy", o+"_yz", o+"_z2", o+"_xz", o+"_x2-y2"]) + if "s" in o: + orb_list.append(o) + elif "p" in o: + orb_list.extend([o+"_y", o+"_z", o+"_x"]) + elif "d" in o: + orb_list.extend([o+"_xy", o+"_yz", o+"_z2", o+"_xz", o+"_x2-y2"]) orbs_per_type[atomtype] = orb_list
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.gitignoredptb/nn/build.pydptb/postprocess/common.pydptb/postprocess/unified/__init__.pydptb/postprocess/unified/calculator.pydptb/postprocess/unified/properties/band.pydptb/postprocess/unified/properties/dos.pydptb/postprocess/unified/system.pydptb/tests/test_postprocess_unified.pyexamples/TBSystems/new_postprocess.ipynb
🧰 Additional context used
📓 Path-based instructions (1)
dptb/tests/**/*.py
📄 CodeRabbit inference engine (GEMINI.md)
Place unit tests in the
dptb/testsdirectory
Files:
dptb/tests/test_postprocess_unified.py
🧠 Learnings (1)
📚 Learning: 2025-11-26T21:32:19.567Z
Learnt from: CR
Repo: deepmodeling/DeePTB PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-26T21:32:19.567Z
Learning: Organize code into a main `dptb` package with subpackages for different functionalities (data, nn, negf, etc.)
Applied to files:
dptb/postprocess/unified/__init__.py
🧬 Code graph analysis (4)
dptb/postprocess/unified/properties/band.py (4)
dptb/postprocess/unified/system.py (5)
data(67-69)TBSystem(18-194)atoms(72-74)calculator(57-59)band_data(88-91)dptb/utils/make_kpoints.py (3)
ase_kpath(370-388)abacus_kpath(308-366)vasp_kpath(390-451)dptb/postprocess/common.py (1)
is_gui_available(115-171)dptb/postprocess/unified/properties/dos.py (4)
kpoints(137-138)plot(30-91)plot(278-279)compute(153-270)
dptb/tests/test_postprocess_unified.py (4)
dptb/postprocess/unified/system.py (7)
TBSystem(18-194)band(81-85)dos(94-98)data(67-69)calculator(57-59)atoms(72-74)atom_orbs(77-78)dptb/postprocess/unified/properties/band.py (4)
BandStructureData(18-144)compute(223-250)plot(47-133)plot(259-261)dptb/postprocess/unified/properties/dos.py (1)
DosData(17-105)dptb/postprocess/unified/calculator.py (4)
get_hk(70-81)get_hk(194-229)get_hr(30-40)get_hr(147-155)
dptb/postprocess/unified/system.py (3)
dptb/postprocess/unified/calculator.py (4)
HamiltonianCalculator(10-86)DeePTBAdapter(88-242)get_orbital_info(84-86)get_orbital_info(231-242)dptb/postprocess/unified/properties/band.py (4)
BandAccessor(146-265)band_data(253-257)set_kpath(177-221)compute(223-250)dptb/postprocess/unified/properties/dos.py (5)
DosAccessor(107-282)dos_data(273-276)compute(153-270)set_kpoints(117-134)set_dos_config(140-151)
dptb/postprocess/unified/calculator.py (4)
dptb/data/AtomicData.py (1)
AtomicData(313-878)dptb/utils/argcheck.py (2)
get_cutoffs_from_model_options(1591-1653)model_options(683-694)dptb/nn/energy.py (2)
Eigenvalues(16-115)Eigh(117-204)dptb/data/interfaces/ham_to_feature.py (1)
feature_to_block(322-415)
🪛 Ruff (0.14.10)
dptb/postprocess/common.py
158-158: Consider moving this statement to an else block
(TRY300)
159-159: Do not use bare except
(E722)
159-160: try-except-continue detected, consider logging the exception
(S112)
166-166: Do not catch blind exception: Exception
(BLE001)
169-169: Do not catch blind exception: Exception
(BLE001)
dptb/postprocess/unified/__init__.py
4-4: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
dptb/postprocess/unified/properties/band.py
212-212: Avoid specifying long messages outside the exception class
(TRY003)
228-228: Avoid specifying long messages outside the exception class
(TRY003)
256-256: Avoid specifying long messages outside the exception class
(TRY003)
dptb/tests/test_postprocess_unified.py
138-138: Unpacked variable sr_blocks is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
dptb/postprocess/unified/properties/dos.py
31-31: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
39-39: Unpacked variable fig is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
158-158: Avoid specifying long messages outside the exception class
(TRY003)
160-160: Avoid specifying long messages outside the exception class
(TRY003)
247-247: Unpacked variable nk is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
247-247: Unpacked variable nb is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
275-275: Avoid specifying long messages outside the exception class
(TRY003)
dptb/postprocess/unified/system.py
32-32: Do not perform function call torch.device in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
46-46: Avoid specifying long messages outside the exception class
(TRY003)
126-126: Prefer TypeError exception for invalid type
(TRY004)
126-126: Avoid specifying long messages outside the exception class
(TRY003)
163-163: Unused method argument: kwargs
(ARG002)
164-164: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
165-165: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
183-183: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
184-184: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
dptb/postprocess/unified/calculator.py
100-100: Avoid specifying long messages outside the exception class
(TRY003)
128-128: Avoid specifying long messages outside the exception class
(TRY003)
167-167: Avoid specifying long messages outside the exception class
(TRY003)
182-182: Avoid specifying long messages outside the exception class
(TRY003)
237-237: Multiple statements on one line (colon)
(E701)
238-238: Multiple statements on one line (colon)
(E701)
239-239: Multiple statements on one line (colon)
(E701)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (11)
dptb/nn/build.py (2)
8-8: LGTM!The
osimport is appropriately added to support the path manipulation logic introduced in lines 47-48.
46-48: No action needed. The checkpoint filesbase_poly2.pthandbase_poly4.pthexist in the repository atdptb/nn/dftb/, and the path construction logic correctly resolves to these files. The code functions as intended.If enhanced error handling with a clearer message is desired, the suggested check can be added, but it is optional—
torch.load()will naturally fail with a descriptive error if the file is missing..gitignore (1)
2-3: LGTM!The new ignore patterns for
*_bands.pngand*_dos.pngappropriately exclude generated plot files from the new band structure and DOS plotting functionality.dptb/postprocess/unified/__init__.py (1)
1-4: LGTM!The package initialization correctly exposes the public API for the unified post-processing interface. The imports and
__all__definition follow standard Python patterns.dptb/tests/test_postprocess_unified.py (1)
19-30: LGTM - Well-structured test fixture.The module-scoped fixture appropriately skips tests when example data files are not available, avoiding CI failures in minimal test environments while allowing integration testing when data is present.
dptb/postprocess/unified/properties/band.py (1)
47-133: LGTM - Robust plotting implementation.The
plotmethod properly handles GUI auto-detection viais_gui_available(), supports reference band overlay, and includes sensible defaults for figure styling. The conditionalplt.show()vsplt.close()logic prevents hanging in headless environments.dptb/postprocess/unified/properties/dos.py (1)
153-192: LGTM - Well-structured DOS/PDOS computation.The
computemethod correctly handles both total DOS and PDOS calculations, including proper treatment of overlap matrices for non-orthogonal basis sets. The eigenvector transpose (line 184) for overlap-aware PDOS is correctly noted with a comment explaining the rationale.dptb/postprocess/unified/system.py (1)
56-98: LGTM - Clean property-based API design.The lazy initialization pattern for
bandanddosaccessors is well-implemented, providing a clean interface for users (system.band.compute()) while deferring object creation until needed.dptb/postprocess/unified/calculator.py (3)
10-86: LGTM - Clean abstract interface design.The
HamiltonianCalculatorABC provides a well-defined contract with clear method signatures and documentation. The separation between eigenvalue-only (get_eigenvalues) and full eigensolver (get_eigenstates) methods is a good design choice for performance optimization.
194-229: LGTM - Robustget_hkimplementation.The
get_hkmethod properly handles k-point injection with appropriate tensor conversion and nested tensor wrapping. The shallow copy ofatomic_dataprevents mutation of the original data when overriding k-points.
172-173: Hard-coded batch index assumes single-structure batches.The
[0]indexing onENERGY_EIGENVALUE_KEYassumes batch size of 1. This pattern is hard-coded in multiple postprocessing entry points (loss calculation, eigenvalue computation, tests), and while the data structure supports batching (n_batchdimension exists), all training configs enforcebatch_size: 1. Consider adding validation at inference entry points or making this constraint explicit in documentation.
| if sys.platform.startswith('linux') or sys.platform.startswith('darwin'): | ||
| display = os.environ.get('DISPLAY') | ||
| if display is None: | ||
| return False |
There was a problem hiding this comment.
macOS does not require DISPLAY for GUI applications.
On macOS (darwin), GUI applications use Quartz/Cocoa, not X11, so DISPLAY is typically unset even when a GUI is available. This check will incorrectly return False on most macOS systems with a working display.
🔎 Proposed fix
# Check DISPLAY environment variable (Unix-like systems)
- if sys.platform.startswith('linux') or sys.platform.startswith('darwin'):
+ if sys.platform.startswith('linux'):
display = os.environ.get('DISPLAY')
if display is None:
return False🤖 Prompt for AI Agents
In dptb/postprocess/common.py around lines 129-132, the current check treats
both linux and darwin the same and returns False when DISPLAY is unset, but
macOS (darwin) does not require DISPLAY; change the logic so only Linux checks
DISPLAY: remove darwin from the platform check (or explicitly check for linux
only), keep the existing behavior for Linux (return False when DISPLAY is None),
and ensure darwin falls through to the normal GUI-available path instead of
returning False.
| self.model.eval() | ||
|
|
||
| # Check model capabilities | ||
| self.overlap = hasattr(model, 'overlap') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how 'overlap' attribute is used in existing model classes
rg -n -A2 -B2 'self\.overlap\s*=' --type=py -g '!*test*' | head -50Repository: deepmodeling/DeePTB
Length of output: 182
🏁 Script executed:
#!/bin/bash
# Also check how self.overlap is used in calculator.py specifically
rg -n 'overlap' dptb/postprocess/unified/calculator.py | head -30Repository: deepmodeling/DeePTB
Length of output: 182
🏁 Script executed:
#!/bin/bash
# Check model definitions and how overlap is set
rg -n 'overlap\s*=' --type=py -g 'model*' | head -30Repository: deepmodeling/DeePTB
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Get broader context around line 98 in calculator.py
head -120 dptb/postprocess/unified/calculator.py | tail -30Repository: deepmodeling/DeePTB
Length of output: 1117
Use getattr() to check the actual value of the overlap attribute, not just its existence.
hasattr(model, 'overlap') returns True if the attribute exists, regardless of its value. If the attribute exists but is False, self.overlap will incorrectly be set to True, causing overlap kwargs to be added when they shouldn't be. Use self.overlap = getattr(model, 'overlap', False) to capture the actual boolean value.
🤖 Prompt for AI Agents
In dptb/postprocess/unified/calculator.py around line 98, the code sets
self.overlap using hasattr(model, 'overlap') which only checks existence and can
return True even when the attribute value is False; change this to read the
actual boolean value using getattr(model, 'overlap', False) so self.overlap
reflects the attribute's value (defaulting to False when missing), and update
any tests or callers if they rely on the previous semantics.
Fix dtype mismatches in tensor product calculations by explicitly setting dtype to match input tensors. Updates J_full_small initialization and _Jd tensor conversion in batch_wigner_D function, and enforces double precision in test cases to prevent numerical precision issues.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dptb/tests/test_tp.py (1)
41-46: LGTM! Wigner D rotation matrices correctly use float64.The
wigner_Dcalls now receive float64 scalar inputs, ensuring the rotation matrices are computed in double precision. This is consistent with the overall goal of enforcing float64 throughout the test.Optional: Static analysis suggests renaming the unused loop variable
pto_pon line 41 to indicate it's intentionally unused, and notes thatl(angular momentum quantum number) could be considered ambiguous. However, these are minor style considerations andlis conventional in physics code.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dptb/nn/tensor_product.pydptb/tests/test_tp.py
🧰 Additional context used
📓 Path-based instructions (1)
dptb/tests/**/*.py
📄 CodeRabbit inference engine (GEMINI.md)
Place unit tests in the
dptb/testsdirectory
Files:
dptb/tests/test_tp.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: deepmodeling/DeePTB PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-26T21:32:19.567Z
Learning: Structure NEGF calculations in the `dptb/negf` subpackage
🧬 Code graph analysis (2)
dptb/tests/test_tp.py (1)
dptb/nn/tensor_product.py (1)
wigner_D(89-100)
dptb/nn/tensor_product.py (2)
dptb/utils/argcheck.py (1)
device(1099-1106)dptb/utils/torch_geometric/data.py (1)
to(316-324)
🪛 Ruff (0.14.10)
dptb/tests/test_tp.py
41-41: Ambiguous variable name: l
(E741)
41-41: Loop control variable p not used within loop body
Rename unused p to _p
(B007)
51-51: Ambiguous variable name: l
(E741)
51-51: Loop control variable p not used within loop body
Rename unused p to _p
(B007)
dptb/nn/tensor_product.py
78-78: Ambiguous variable name: l
(E741)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
dptb/tests/test_tp.py (3)
26-30: LGTM! Double precision correctly enforced.The addition of
.double()ensures the SO2_Linear module operates in float64 precision, which is appropriate for numerical accuracy in rotation equivariance tests.
31-38: LGTM! Input tensors correctly use float64.All input tensors (
a,R,vec) are now consistently created withdtype=torch.float64, ensuring double precision throughout the test.
48-55: LGTM! Test assertions and tolerance are appropriate.The final
wigner_Dcall (line 52) correctly uses float64, and the assertion tolerance of5e-5is appropriate for double-precision rotation equivariance testing. The test logic correctly verifies SO2 rotation symmetry.dptb/nn/tensor_product.py (1)
77-80: LGTM! Explicit dtype handling improves precision consistency.The explicit dtype specification for
J_full_smalland the dtype conversion of_Jd[l]blocks ensure that all tensor operations maintain consistent precision with the inputalpha. This prevents implicit conversions and potential precision loss in Wigner-D matrix computations, which is critical for numerical accuracy in quantum mechanical calculations.Note: The Ruff warning about variable name
lat line 78 is a false positive—lis standard notation for angular momentum quantum numbers in physics/mathematics code and should be retained for domain clarity.
- Initialize _band_data attribute in BandAccessor.__init__ for future use - Remove NestedTensor type check in test_postprocess_unified.py to simplify validation - Ensure consistent tensor type handling in Hamiltonian calculations
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
dptb/tests/test_postprocess_unified.py (1)
133-154: Consider prefixing unused variable with underscore.The
sr_blocksvariable is unpacked but not used in the test. To silence the static analysis warning and signal intent, prefix it with an underscore.🔎 Proposed fix
- hr_blocks, sr_blocks = tbsys.calculator.get_hr(tbsys.data) + hr_blocks, _sr_blocks = tbsys.calculator.get_hr(tbsys.data)Based on static analysis hints.
dptb/postprocess/unified/properties/band.py (1)
146-154: Remove duplicate docstring.Lines 147-154 repeat the same docstring. Keep only one copy.
🔎 Proposed fix
class BandAccessor: """ Accessor for Band Structure functionality on a TBSystem. Allows syntax like: system.band.set_kpath(...) """ - """ - Accessor for Band Structure functionality on a TBSystem. - Allows syntax like: system.band.set_kpath(...) - """ def __init__(self, system: 'TBSystem'):
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dptb/postprocess/unified/properties/band.pydptb/tests/test_postprocess_unified.py
🧰 Additional context used
📓 Path-based instructions (1)
dptb/tests/**/*.py
📄 CodeRabbit inference engine (GEMINI.md)
Place unit tests in the
dptb/testsdirectory
Files:
dptb/tests/test_postprocess_unified.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: deepmodeling/DeePTB PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-26T21:32:19.567Z
Learning: Organize code into a main `dptb` package with subpackages for different functionalities (data, nn, negf, etc.)
Learnt from: CR
Repo: deepmodeling/DeePTB PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-26T21:32:19.567Z
Learning: Structure NEGF calculations in the `dptb/negf` subpackage
🧬 Code graph analysis (1)
dptb/tests/test_postprocess_unified.py (4)
dptb/postprocess/unified/system.py (7)
TBSystem(18-194)band(81-85)dos(94-98)data(67-69)calculator(57-59)atoms(72-74)atom_orbs(77-78)dptb/postprocess/unified/properties/band.py (3)
BandStructureData(18-144)plot(47-133)plot(260-262)dptb/postprocess/unified/properties/dos.py (1)
DosData(17-105)dptb/postprocess/unified/calculator.py (4)
get_hk(70-81)get_hk(194-229)get_hr(30-40)get_hr(147-155)
🪛 Ruff (0.14.10)
dptb/tests/test_postprocess_unified.py
138-138: Unpacked variable sr_blocks is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
dptb/postprocess/unified/properties/band.py
213-213: Avoid specifying long messages outside the exception class
(TRY003)
229-229: Avoid specifying long messages outside the exception class
(TRY003)
257-257: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (9)
dptb/tests/test_postprocess_unified.py (6)
1-18: LGTM! Clean imports and path setup.The test imports and path configuration are well-structured for integration testing with example data.
19-31: LGTM! Efficient fixture design.Module-scoped fixture appropriately avoids repeated model loading, and gracefully skips when example data is unavailable.
32-43: LGTM! Comprehensive initialization checks.The test appropriately validates TBSystem setup and key properties.
44-72: LGTM! Thorough band calculation test.The test appropriately exercises the band structure workflow, validates results, and cleans up generated files.
73-97: LGTM! Complete DOS calculation test.The test validates both total DOS and PDOS dimensions, correctly verifying orbital correspondence.
98-132: LGTM! Robust Hamiltonian testing.The test correctly handles both regular and nested tensor cases, and validates dimensions against orbital counts. The shape assertions appropriately verify multi-k-point results.
dptb/postprocess/unified/properties/band.py (3)
1-17: LGTM! Clean imports and TYPE_CHECKING usage.The imports are well-organized, and the TYPE_CHECKING pattern correctly avoids circular dependencies with TBSystem.
18-145: LGTM! Well-designed data class with comprehensive plotting.BandStructureData appropriately encapsulates band results and provides flexible plotting with automatic GUI detection and reference band comparison.
155-267: LGTM! Flexible and well-structured band accessor.The BandAccessor provides comprehensive k-path configuration options (ase, abacus, vasp, array methods), appropriate error handling, and clean compute/access workflow. The nested tensor handling for k-points (line 222) correctly interfaces with the model's expected format.
Add comprehensive tutorial demonstrating how to export DeePTB models to PythTB format. The notebook covers loading pre-trained models, calculating band structures with both DeePTB and PythTB, and comparing results to ensure compatibility. This enables users to leverage external tools for post-processing trained tight-binding models.
Add proper Fermi level calculation functionality to replace hardcoded values. The band structure and DOS properties now use the system's Fermi level when available, falling back to 0.0 when not set. Includes a new utility function for calculating Fermi energy from eigenvalues using various smearing methods and temperature parameters. Also improves DOS plotting with inward tick direction.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
dptb/utils/make_kpoints.py (1)
128-140: Dead code and mutable default argument.
Line 133 computes
kpointsusingnp.indices, but this value is immediately overwritten in theif/elseblock (lines 135-138). This line is dead code.The mutable default
meshgrid=[1,1,1]should be replaced withNoneper Python best practices.🔎 Proposed fix
-def kmesh_sampling_weight(meshgrid=[1,1,1], is_gamma_center=True): +def kmesh_sampling_weight(meshgrid=None, is_gamma_center=True): """ Generate k-points using Monkhorst-Pack method based on given meshgrid. The k-points are centered at Gamma point by default. """ - - kpoints = np.indices(meshgrid).transpose((1, 2, 3, 0)).reshape((-1, 3)) + if meshgrid is None: + meshgrid = [1, 1, 1] if is_gamma_center: kpoints = gamma_center(meshgrid) else: kpoints = monkhorst_pack(meshgrid) weights = np.ones(kpoints.shape[0]) return kpoints, weightsdptb/tests/test_postprocess_unified_fermi.py (1)
1-8: Unused import.
torchis imported on line 5 but never used in this test file.🔎 Proposed fix
import pytest import os import numpy as np -import torch from dptb.postprocess.unified.system import TBSystem from dptb.postprocess.unified.utils import calculate_fermi_level from dptb.data import AtomicDataDictdptb/postprocess/unified/utils.py (1)
69-72: PotentialNaNifq_tolis unexpectedly large.Line 70 computes
np.sqrt(-np.log(q_tol * 1e-2)). Ifq_tol >= 100, the argument tosqrtbecomes non-positive, resulting inNaNfordrange. While the defaultq_tol=1e-10is safe, consider adding input validation.🔎 Proposed fix
+ if q_tol <= 0 or q_tol >= 1: + raise ValueError("q_tol must be in the range (0, 1)") + # Expand search range to ensure Ef is within bounds even if it's in the band gap drange = kT * np.sqrt(-np.log(q_tol * 1e-2))dptb/postprocess/unified/properties/band.py (2)
146-154: Duplicate docstring forBandAccessorclass.The class docstring is duplicated on lines 147-150 and 151-154.
🔎 Proposed fix
class BandAccessor: """ Accessor for Band Structure functionality on a TBSystem. Allows syntax like: system.band.set_kpath(...) """ - """ - Accessor for Band Structure functionality on a TBSystem. - Allows syntax like: system.band.set_kpath(...) - """ def __init__(self, system: 'TBSystem'):
75-89: Reference band alignment may fail with mismatched shapes.If
ref_bandshas a different number of k-points thanself.eigenvalues, the element-wise operations and downsampling at line 88 could produce unexpected results or errors. Consider adding a shape check.🔎 Proposed fix
if ref_bands is not None: if isinstance(ref_bands, str): ref_data = np.load(ref_bands) else: ref_data = ref_bands + if ref_data.shape[0] != self.eigenvalues.shape[0]: + log.warning(f"Reference bands have {ref_data.shape[0]} k-points but computed bands have {self.eigenvalues.shape[0]}. Alignment may be inaccurate.") + # Simple realignment logic (shift to align mins) # Note: A more robust alignment might be needed in future shift = np.min(self.eigenvalues) - np.min(ref_data)dptb/postprocess/unified/properties/dos.py (3)
140-142: Use explicit exception instead ofassertfor input validation.
assertstatements are stripped when Python runs with optimization flags (-O), so the validation on line 142 could be bypassed in production.🔎 Proposed fix
def set_dos_config(self, erange, npts, smearing='gaussian', sigma=0.05, pdos=False, **kwargs): # Update processing config - assert smearing in ['gaussian','lorentzian'], "The smearing should be either 'gaussian' or 'lorentzian' !" + if smearing not in ['gaussian', 'lorentzian']: + raise ValueError("smearing must be either 'gaussian' or 'lorentzian'") self._config.update({
211-221: Broadening function silently returns 0 for unknown method.If an unknown smearing method reaches the internal
broadeningfunction (e.g., due to future code changes or typos), it silently returns 0, causing subtle bugs. However, sinceset_dos_configvalidates the method, this is a defensive concern.🔎 Proposed fix
def broadening(E_grid, E_vals, sigma, method): # E_grid: [N_E, 1] # E_vals: [1, N_states] delta = E_grid[:, None] - E_vals[None, :] if method == 'gaussian': return np.exp(-0.5 * (delta / sigma)**2) / (np.sqrt(2 * np.pi) * sigma) elif method == 'lorentzian': return (1 / np.pi) * (sigma / (delta**2 + sigma**2)) else: - return 0 + raise ValueError(f"Unknown broadening method: {method}")
30-31: Add explicitOptionalforselected_orbitalsparameter.Per PEP 484, the
= Nonedefault should useOptional[...]type annotation.🔎 Proposed fix
- def plot(self, filename: Optional[str] = 'dos.png', show: Optional[bool] = None, - xlim: Optional[List[float]] = None, plot_pdos: bool = False, selected_orbitals: Union[List[int], List[str]] = None): + def plot(self, filename: Optional[str] = 'dos.png', show: Optional[bool] = None, + xlim: Optional[List[float]] = None, plot_pdos: bool = False, + selected_orbitals: Optional[Union[List[int], List[str]]] = None):dptb/postprocess/unified/system.py (4)
29-35: Avoid function call in default argument.
torch.device("cpu")is called at function definition time. While this is less problematic than mutable defaults, it's still best practice to useNoneand handle it inside the function.🔎 Proposed fix
def __init__( self, data: Union[AtomicData, ase.Atoms, str], calculator: Union[HamiltonianCalculator, torch.nn.Module, str], override_overlap: Optional[str] = None, - device: Optional[Union[str, torch.device]]= torch.device("cpu") + device: Optional[Union[str, torch.device]] = None ): # Initialize Calculator/Model + if device is None: + device = torch.device("cpu") if isinstance(calculator, str):
88-94: Use logging or raise exception instead ofprint()for missing electrons.Using
print()in library code is poor practice—it bypasses the logging system and cannot be suppressed by users. Either raise an exception or uselog.warning().🔎 Proposed fix
@property def total_electrons(self): if self._total_electrons is None: - print('Please call set_electrons first!') - else: - return self._total_electrons + raise RuntimeError("Total electrons not set. Call set_electrons() first.") + return self._total_electrons
202-206: Useraise ... from eto preserve exception chain.When re-raising a different exception, use
from eto maintain the traceback chain for debugging.🔎 Proposed fix
try: self._total_electrons = np.array([nel_atom[s] for s in self.atomic_symbols]).sum() except KeyError as e: - raise KeyError(f"Element {e} found in system but not in nel_atom dictionary: {nel_atom}") + raise KeyError(f"Element {e} found in system but not in nel_atom dictionary: {nel_atom}") from e
279-291: Unusedkwargsparameter.The
kwargsparameter on line 279 is captured but never used. Either remove it or document its intended purpose for future extensions.🔎 Proposed fix
- def get_bands(self, kpath_config: Optional[dict] = None, reuse: Optional[bool]=True, **kwargs): + def get_bands(self, kpath_config: Optional[dict] = None, reuse: Optional[bool]=True):
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
dptb/postprocess/unified/properties/band.pydptb/postprocess/unified/properties/dos.pydptb/postprocess/unified/system.pydptb/postprocess/unified/utils.pydptb/tests/test_postprocess_unified_fermi.pydptb/utils/make_kpoints.pyexamples/TBSystems/new_postprocess.ipynbexamples/ToPythTB/topythtb.ipynb
🧰 Additional context used
📓 Path-based instructions (1)
dptb/tests/**/*.py
📄 CodeRabbit inference engine (GEMINI.md)
Place unit tests in the
dptb/testsdirectory
Files:
dptb/tests/test_postprocess_unified_fermi.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: deepmodeling/DeePTB PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-26T21:32:19.567Z
Learning: Structure NEGF calculations in the `dptb/negf` subpackage
Learnt from: CR
Repo: deepmodeling/DeePTB PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-26T21:32:19.567Z
Learning: Organize code into a main `dptb` package with subpackages for different functionalities (data, nn, negf, etc.)
🧬 Code graph analysis (3)
dptb/postprocess/unified/utils.py (2)
dptb/plugins/train_logger.py (1)
log(29-30)dptb/postprocess/unified/system.py (1)
total_electrons(89-93)
dptb/postprocess/unified/properties/band.py (4)
dptb/postprocess/unified/system.py (3)
data(71-73)TBSystem(20-309)calculator(61-63)dptb/utils/make_kpoints.py (3)
ase_kpath(382-400)abacus_kpath(320-378)vasp_kpath(402-463)dptb/postprocess/common.py (1)
is_gui_available(115-171)dptb/postprocess/unified/calculator.py (2)
get_eigenvalues(43-53)get_eigenvalues(157-173)
dptb/tests/test_postprocess_unified_fermi.py (2)
dptb/postprocess/unified/system.py (2)
total_electrons(89-93)set_electrons(184-205)dptb/postprocess/unified/utils.py (1)
calculate_fermi_level(29-130)
🪛 Ruff (0.14.10)
dptb/postprocess/unified/utils.py
107-107: Avoid specifying long messages outside the exception class
(TRY003)
dptb/postprocess/unified/properties/band.py
213-213: Avoid specifying long messages outside the exception class
(TRY003)
229-229: Avoid specifying long messages outside the exception class
(TRY003)
261-261: Avoid specifying long messages outside the exception class
(TRY003)
dptb/postprocess/unified/system.py
34-34: Do not perform function call torch.device in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
48-48: Avoid specifying long messages outside the exception class
(TRY003)
147-147: Prefer TypeError exception for invalid type
(TRY004)
147-147: Avoid specifying long messages outside the exception class
(TRY003)
200-200: Avoid specifying long messages outside the exception class
(TRY003)
205-205: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
205-205: Avoid specifying long messages outside the exception class
(TRY003)
279-279: Unused method argument: kwargs
(ARG002)
280-280: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
281-281: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
298-298: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
299-299: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
dptb/utils/make_kpoints.py
128-128: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
dptb/postprocess/unified/properties/dos.py
31-31: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
39-39: Unpacked variable fig is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
158-158: Avoid specifying long messages outside the exception class
(TRY003)
160-160: Avoid specifying long messages outside the exception class
(TRY003)
251-251: Unpacked variable nk is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
251-251: Unpacked variable nb is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
279-279: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
dptb/postprocess/unified/utils.py (2)
7-19: LGTM!The Fermi-Dirac smearing implementation correctly handles numerical edge cases with masking for extreme
xvalues, avoiding overflow/underflow. The formula1/(expm1(x)+2) = 1/(e^x+1)is the correct FD occupation function.
29-56: Clear and well-documented function signature.The docstring comprehensively covers all parameters, return values, and their meanings. The bisection approach with adaptive range expansion is appropriate for this problem.
dptb/postprocess/unified/properties/band.py (1)
155-161: LGTM!The
__init__correctly initializes all private state including_band_data = None, ensuring the property check on line 260 will work as intended.
| if self._system._efermi is None: | ||
| efermi = 0.0 | ||
| log.info('The efermi is not unknown, set it to 0.0!') | ||
| else: |
There was a problem hiding this comment.
Typo in log message: "is not unknown" should be "is unknown" or "is not set".
The current message "The efermi is not unknown" has the opposite meaning from what's intended.
🔎 Proposed fix
if self._system._efermi is None:
efermi = 0.0
- log.info('The efermi is not unknown, set it to 0.0!')
+ log.info('The efermi is unknown, setting it to 0.0!')
else:
efermi = self._system._efermi📝 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.
| if self._system._efermi is None: | |
| efermi = 0.0 | |
| log.info('The efermi is not unknown, set it to 0.0!') | |
| else: | |
| if self._system._efermi is None: | |
| efermi = 0.0 | |
| log.info('The efermi is unknown, setting it to 0.0!') | |
| else: |
🤖 Prompt for AI Agents
In dptb/postprocess/unified/properties/band.py around lines 240 to 243, the log
message "The efermi is not unknown, set it to 0.0!" contains a typo and conveys
the opposite meaning; change the message to something correct and clear such as
"The efermi is unknown, set it to 0.0!" or "The efermi is not set, setting it to
0.0!" while leaving the efermi = 0.0 assignment intact.
| @property | ||
| def dos_data(self): | ||
| if self._dos_data is None: | ||
| raise RuntimeError("DOS not calculated. Call calculate_dos() first.") | ||
| return self._dos_data |
There was a problem hiding this comment.
Inconsistent method name in error message.
The error message references calculate_dos() but the method is named compute().
🔎 Proposed fix
@property
def dos_data(self):
if self._dos_data is None:
- raise RuntimeError("DOS not calculated. Call calculate_dos() first.")
+ raise RuntimeError("DOS not calculated. Call compute() first.")
return self._dos_data📝 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.
| @property | |
| def dos_data(self): | |
| if self._dos_data is None: | |
| raise RuntimeError("DOS not calculated. Call calculate_dos() first.") | |
| return self._dos_data | |
| @property | |
| def dos_data(self): | |
| if self._dos_data is None: | |
| raise RuntimeError("DOS not calculated. Call compute() first.") | |
| return self._dos_data |
🧰 Tools
🪛 Ruff (0.14.10)
279-279: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In dptb/postprocess/unified/properties/dos.py around lines 276 to 280, the
RuntimeError message refers to a non-existent calculate_dos() method; update the
error text to reference the actual method name compute() (e.g., "DOS not
calculated. Call compute() first.") so the message is consistent with the
codebase and guides callers correctly.
|
|
||
| calculated_efermi = self.estimate_efermi_e( | ||
| eigenvalues=eigs.detach().numpy(), | ||
| temperature = temperature, |
There was a problem hiding this comment.
Missing .cpu() before .numpy() may fail on GPU tensors.
If the calculator runs on GPU, eigs.detach().numpy() will fail. Add .cpu() for device-agnostic code.
🔎 Proposed fix
- calculated_efermi = self.estimate_efermi_e(
- eigenvalues=eigs.detach().numpy(),
+ calculated_efermi = self.estimate_efermi_e(
+ eigenvalues=eigs.detach().cpu().numpy(),
temperature = temperature,📝 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.
| calculated_efermi = self.estimate_efermi_e( | |
| eigenvalues=eigs.detach().numpy(), | |
| temperature = temperature, | |
| calculated_efermi = self.estimate_efermi_e( | |
| eigenvalues=eigs.detach().cpu().numpy(), | |
| temperature = temperature, |
🤖 Prompt for AI Agents
In dptb/postprocess/unified/system.py around lines 226 to 229, the code calls
eigs.detach().numpy() which will raise an error if eigs is on GPU; change this
to eigs.detach().cpu().numpy() so the tensor is moved to CPU before converting
to a NumPy array, ensuring device-agnostic behavior and preventing GPU-to-NumPy
failures.
| # Test with weights (half weight for first 5 states) | ||
| weights = np.ones(10) | ||
| weights[:5] = 0.5 # [-5..-1] have weight 0.5. Occupying them gives 2.5 electrons (spin=1) -> 5 electrons (spin=2) | ||
| # Target 4 electrons. | ||
| # occ of -5 (0.5), -4 (0.5), -3 (0.5), -2 (0.5). Sum = 2.0 (state weight) * 2 (spin) = 4.0 electrons. | ||
| # So Ef should be just above -2. | ||
|
|
||
| ef_weighted = calculate_fermi_level( | ||
| eigenvalues=eigenvalues, | ||
| total_electrons=4.0, | ||
| spindeg=2, | ||
| weights=weights, | ||
| temperature=0.01, | ||
| smearing_method='FD' | ||
| ) | ||
| print(f"Weighted Synthetic Ef: {ef_weighted}") | ||
| assert -2.0 <= ef_weighted <= -1.0 |
There was a problem hiding this comment.
Shape mismatch between eigenvalues and weights in second test scenario.
The eigenvalues array has shape (1, 10) (1 k-point, 10 bands), but weights is set to shape (10,). The calculate_fermi_level function (lines 88-90 in utils.py) only reshapes weights when eigenvalues.shape[0] == weights.shape[0], which won't be true here (1 ≠ 10).
This causes a broadcasting mismatch in (weights * occupation).sum(). The test comment suggests treating the 10 values as individual states with different weights, but that's not how the function is designed—it expects k-point weights, not per-band weights.
🔎 Proposed fix to align with k-point weighting semantics
If you want to test non-uniform k-point weights, you'd need multiple k-points:
# Test with weights (half weight for first 5 states)
- weights = np.ones(10)
- weights[:5] = 0.5 # [-5..-1] have weight 0.5. Occupying them gives 2.5 electrons (spin=1) -> 5 electrons (spin=2)
- # Target 4 electrons.
- # occ of -5 (0.5), -4 (0.5), -3 (0.5), -2 (0.5). Sum = 2.0 (state weight) * 2 (spin) = 4.0 electrons.
- # So Ef should be just above -2.
+ # Test with multiple k-points having different weights
+ eigenvalues_multi = np.array([[-5, -4, -3, -2, -1], [0, 1, 2, 3, 4]], dtype=float) # 2 k-points, 5 bands
+ weights_multi = np.array([0.3, 0.7]) # k-point weights
ef_weighted = calculate_fermi_level(
- eigenvalues=eigenvalues,
- total_electrons=4.0,
+ eigenvalues=eigenvalues_multi,
+ total_electrons=4.0,
spindeg=2,
- weights=weights,
+ weights=weights_multi,
temperature=0.01,
smearing_method='FD'
)🤖 Prompt for AI Agents
In dptb/tests/test_postprocess_unified_fermi.py around lines 50–66 there is a
shape mismatch: eigenvalues has shape (1,10) but weights is (10,), and
calculate_fermi_level expects k-point weights (one weight per k-point) or a
weights array matching eigenvalues shape. Fix by either (A) convert the test to
use k-point weights (make weights length 1, e.g. a single value representing the
k-point weight and adjust expected Ef), or (B) if you intended per-band/state
weights, reshape the weights to match eigenvalues into shape (1,10) before
calling calculate_fermi_level (and update the expected Ef/assertion
accordingly).
Add ExportAccessor class to
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
dptb/postprocess/unified/system.py (1)
237-241: Missing.cpu()before.numpy()may fail on GPU tensors.If the calculator runs on GPU,
eigs.detach().numpy()on line 238 will fail. Add.cpu()for device-agnostic code.🔎 Proposed fix
calculated_efermi = self.estimate_efermi_e( - eigenvalues=eigs.detach().numpy(), + eigenvalues=eigs.detach().cpu().numpy(), temperature = temperature, smearing_method=smearing_method, q_tol = q_tol, **kwargs)
🧹 Nitpick comments (6)
dptb/postprocess/unified/system.py (5)
35-35: Move function call out of default argument.The default argument
torch.device("cpu")is evaluated at function definition time. Whiletorch.deviceis typically safe, it's better practice to useNoneas the default and construct the device inside the function body.🔎 Proposed fix
def __init__( self, data: Union[AtomicData, ase.Atoms, str], calculator: Union[HamiltonianCalculator, torch.nn.Module, str], override_overlap: Optional[str] = None, - device: Optional[Union[str, torch.device]]= torch.device("cpu") + device: Optional[Union[str, torch.device]] = None ): + if device is None: + device = torch.device("cpu") # Initialize Calculator/Model
90-95: Replacelog.warning.The
🔎 Proposed fix
@property def total_electrons(self): if self._total_electrons is None: - print('Please call set_electrons first!') + log.warning('total_electrons accessed before set_electrons was called') + return None else: return self._total_electrons
212-215: Add exception chaining withfrom e.When re-raising exceptions within an except clause, use
raise ... from eto preserve the exception chain for better debugging.🔎 Proposed fix
try: self._total_electrons = np.array([nel_atom[s] for s in self.atomic_symbols]).sum() except KeyError as e: - raise KeyError(f"Element {e} found in system but not in nel_atom dictionary: {nel_atom}") + raise KeyError(f"Element {e} found in system but not in nel_atom dictionary: {nel_atom}") from e
289-291: Replace Chinese comments with English and consider removing unusedkwargs.The comments on lines 290-291 use Chinese characters. For an international codebase, use English comments. Additionally, the
kwargsparameter is unused.🔎 Proposed fix
- def get_bands(self, kpath_config: Optional[dict] = None, reuse: Optional[bool]=True, **kwargs): - # 计算能带，返回 bands - # bands 应该是一个类，也有属性。bands.kpoints, bands.eigenvalues, bands.klabels, bands.kticks, 也有函数 bands.plot() + def get_bands(self, kpath_config: Optional[dict] = None, reuse: Optional[bool]=True): + # Compute band structure and return bands accessor + # bands has properties: kpoints, eigenvalues, klabels, kticks, and methods like plot() if self.has_bands and reuse:
303-309: Add proper docstring and replace Chinese comments with English.The docstring on line 306 is a placeholder, and the comments on lines 308-309 use Chinese characters. Provide a complete docstring and use English for comments.
🔎 Example docstring and comment fix
def get_dos(self, kmesh: Optional[Union[list,np.ndarray]] = None, is_gamma_center: Optional[bool] = True, erange: Optional[Union[list,np.ndarray]] = None, npts: Optional[int] = 100, smearing: Optional[str] = 'gaussian', sigma: Optional[float] = 0.05, pdos: Optional[bool]=False, reuse: Optional[bool]=True, **kwargs): """ - docstring, to be added! + Calculate density of states and return DOS accessor. + + Parameters + ---------- + kmesh : list or np.ndarray + K-point mesh for DOS calculation. + is_gamma_center : bool, default True + Whether to use Gamma-centered mesh. + erange : list or np.ndarray, optional + Energy range [emin, emax] for DOS. + npts : int, default 100 + Number of points in DOS grid. + smearing : str, default 'gaussian' + Smearing method. + sigma : float, default 0.05 + Smearing width. + pdos : bool, default False + Calculate projected DOS. + reuse : bool, default True + Reuse existing DOS if available. + + Returns + ------- + DosAccessor + DOS accessor with properties and methods. """ - # 计算态密度，返回 dos - # dos 应该是一个类，也有属性。dos.kmesh, dos.eigenvalues, dos.klabels, dos.kticks, 也有函数 dos.plot() + # Compute DOS and return accessor + # DOS has properties: kmesh, eigenvalues, and methods like plot()dptb/tests/test_export_unified.py (1)
119-120: Use underscore for unused unpacked variable.The
argsvariable on line 119 is unpacked but never used. Prefix it with an underscore to indicate it's intentionally unused.🔎 Proposed fix
- args, kwargs = mock_exporter.write_hr.call_args + _, kwargs = mock_exporter.write_hr.call_args assert kwargs['filename'] == "test_w90_hr.dat"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
dptb/postprocess/unified/properties/export.pydptb/postprocess/unified/system.pydptb/tests/test_export_unified.py
🧰 Additional context used
📓 Path-based instructions (1)
dptb/tests/**/*.py
📄 CodeRabbit inference engine (GEMINI.md)
Place unit tests in the
dptb/testsdirectory
Files:
dptb/tests/test_export_unified.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: deepmodeling/DeePTB PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-26T21:32:19.567Z
Learning: Organize code into a main `dptb` package with subpackages for different functionalities (data, nn, negf, etc.)
Learnt from: CR
Repo: deepmodeling/DeePTB PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-26T21:32:19.567Z
Learning: Structure NEGF calculations in the `dptb/negf` subpackage
🪛 Ruff (0.14.10)
dptb/tests/test_export_unified.py
119-119: Unpacked variable args is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
dptb/postprocess/unified/system.py
35-35: Do not perform function call torch.device in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
49-49: Avoid specifying long messages outside the exception class
(TRY003)
157-157: Prefer TypeError exception for invalid type
(TRY004)
157-157: Avoid specifying long messages outside the exception class
(TRY003)
210-210: Avoid specifying long messages outside the exception class
(TRY003)
215-215: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
215-215: Avoid specifying long messages outside the exception class
(TRY003)
289-289: Unused method argument: kwargs
(ARG002)
290-290: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
291-291: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
308-308: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
309-309: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
| def to_pybinding(self, results_path: Optional[str] = None): | ||
| """ | ||
| Export to Pybinding Lattice. | ||
| Args: | ||
| results_path: Optional path to save results. | ||
| Returns: | ||
| pybinding.Lattice: The Pybinding lattice object. | ||
| """ | ||
| # Overlap check is handled in ToPybinding | ||
| exporter = ToPybinding( | ||
| model=self._system.model, | ||
| results_path=results_path, | ||
| overlap=False, # Pybinding doesn't support overlap usually | ||
| device=self._system.calculator.device | ||
| ) | ||
| return exporter.get_lattice(self._system._atomic_data) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Pybinding overlap matrix support tight-binding
💡 Result:
Short answer: pybinding does not provide built‑in support for a non‑orthogonal overlap (S) matrix — it constructs a tight‑binding Hamiltonian (Model.hamiltonian) only. You can, however, assemble an overlap matrix yourself (or use another package that exposes S) and then solve the generalized eigenproblem A x = λ S x with SciPy. [1][2][3][4]
References and practical notes
- pybinding Model/hamiltonian is a sparse Hamiltonian (scipy.sparse.csr_matrix); no S/overlap API is documented. [1]
- If you need libraries that expose both H and S, consider alternatives such as sisl (has H and S attributes) or PySKTB for Slater–Koster TB workflows. [3][4]
- To solve a generalized problem once you have H and S: use scipy.sparse.linalg.eigsh / scipy.sparse.linalg.eigs (pass S as the M argument) or scipy.linalg.eig/eigh for dense matrices. Example: scipy.sparse.linalg.eigsh(H, k=..., M=S, ...). See SciPy docs for details and constraints (M must be Hermitian/positive(semi)definite etc.). [2]
If you want, I can:
- show a minimal code snippet assembling a sparse S and solving H x = λ S x with SciPy, or
- show how to extract H from a pybinding Model and combine it with an externally built S. Which do you prefer?
🏁 Script executed:
# Search for the ToPybinding class implementation
fd -t f "ToPybinding" --type f -x grep -l "class ToPybinding" {}Repository: deepmodeling/DeePTB
Length of output: 45
🏁 Script executed:
# Also search for references to overlap in the exporter classes
rg -t py "overlap" --max-count 20 -B 2 -A 2Repository: deepmodeling/DeePTB
Length of output: 50376
🏁 Script executed:
# Find ToPybinding class definition
find . -type f -name "*.py" -exec grep -l "class ToPybinding" {} \;Repository: deepmodeling/DeePTB
Length of output: 96
🏁 Script executed:
# Search for the ToPybinding file
fd -type f -name "*pybinding*"Repository: deepmodeling/DeePTB
Length of output: 233
🏁 Script executed:
# Search for ToPybinding in postprocess/unified
find dptb/postprocess/unified -type f -name "*.py" -exec grep -l "ToPybinding" {} \;Repository: deepmodeling/DeePTB
Length of output: 108
🏁 Script executed:
cat -n dptb/postprocess/topybinding.py | head -150Repository: deepmodeling/DeePTB
Length of output: 3948
🏁 Script executed:
# Also search for how ToPybinding handles the overlap parameter
rg -A 5 -B 5 "def __init__" dptb/postprocess/topybinding.py | head -40Repository: deepmodeling/DeePTB
Length of output: 402
Hardcoding overlap=False is appropriate given Pybinding's limitations, but consider exposing it as a parameter.
Pybinding does not support overlap matrices—it only constructs the Hamiltonian matrix. The hardcoded overlap=False on line 46 is necessary. However, you could optionally expose overlap as a parameter to to_pybinding() and let the underlying ToPybinding class handle the validation (which already raises NotImplementedError if overlap is True). This would provide clearer error messaging if users attempt to export with overlap enabled. Alternatively, document this limitation in the method docstring.
🤖 Prompt for AI Agents
In dptb/postprocess/unified/properties/export.py around lines 32 to 49, the
to_pybinding method currently hardcodes overlap=False when constructing
ToPybinding; change the signature to accept an optional overlap: bool = False
parameter, forward that parameter to ToPybinding instead of hardcoding, and
update the docstring to note that pybinding does not support overlap and that
the underlying ToPybinding will raise NotImplementedError if overlap=True; this
exposes the option to callers while preserving existing behavior by default.
| # Assuming effer from system if available, else 0.0 | ||
| e_fermi = self._system.efermi if self._system._efermi is not None else 0.0 | ||
| return exporter.get_cell(self._system._atomic_data, e_fermi=e_fermi) |
There was a problem hiding this comment.
Direct property access may raise unexpected assertion.
Line 68 accesses self._system.efermi which will raise an assertion error if efermi is not set (see line 125 in system.py). Use self._system._efermi directly to allow the fallback to 0.0.
🔎 Proposed fix
- # Assuming effer from system if available, else 0.0
- e_fermi = self._system.efermi if self._system._efermi is not None else 0.0
+ # Use efermi from system if available, else 0.0
+ e_fermi = self._system._efermi if self._system._efermi is not None else 0.0
return exporter.get_cell(self._system._atomic_data, e_fermi=e_fermi)📝 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.
| # Assuming effer from system if available, else 0.0 | |
| e_fermi = self._system.efermi if self._system._efermi is not None else 0.0 | |
| return exporter.get_cell(self._system._atomic_data, e_fermi=e_fermi) | |
| # Use efermi from system if available, else 0.0 | |
| e_fermi = self._system._efermi if self._system._efermi is not None else 0.0 | |
| return exporter.get_cell(self._system._atomic_data, e_fermi=e_fermi) |
🤖 Prompt for AI Agents
In dptb/postprocess/unified/properties/export.py around lines 67 to 69, avoid
calling the property self._system.efermi because it asserts if efermi is unset;
instead read the underlying attribute self._system._efermi and use it with a
fallback to 0.0 (e_fermi = self._system._efermi if self._system._efermi is not
None else 0.0) before returning exporter.get_cell(self._system._atomic_data,
e_fermi=e_fermi).
| exporter = ToWannier90(self._system.model, device=self._system.calculator.device) | ||
|
|
||
| e_fermi = self._system.efermi if self._system._efermi is not None else 0.0 | ||
|
|
||
| exporter.write_hr( | ||
| self._system._atomic_data, | ||
| filename=f"{filename_prefix}_hr.dat", | ||
| e_fermi=e_fermi | ||
| ) |
There was a problem hiding this comment.
Direct property access may raise unexpected assertion.
Line 80 has the same issue as to_tbplas: accessing self._system.efermi will raise an assertion error if efermi is not set. Use self._system._efermi directly.
🔎 Proposed fix
exporter = ToWannier90(self._system.model, device=self._system.calculator.device)
- e_fermi = self._system.efermi if self._system._efermi is not None else 0.0
+ e_fermi = self._system._efermi if self._system._efermi is not None else 0.0
exporter.write_hr(📝 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.
| exporter = ToWannier90(self._system.model, device=self._system.calculator.device) | |
| e_fermi = self._system.efermi if self._system._efermi is not None else 0.0 | |
| exporter.write_hr( | |
| self._system._atomic_data, | |
| filename=f"{filename_prefix}_hr.dat", | |
| e_fermi=e_fermi | |
| ) | |
| exporter = ToWannier90(self._system.model, device=self._system.calculator.device) | |
| e_fermi = self._system._efermi if self._system._efermi is not None else 0.0 | |
| exporter.write_hr( | |
| self._system._atomic_data, | |
| filename=f"{filename_prefix}_hr.dat", | |
| e_fermi=e_fermi | |
| ) |
🤖 Prompt for AI Agents
In dptb/postprocess/unified/properties/export.py around lines 78 to 86, the code
reads self._system.efermi which can raise an assertion if efermi property
enforces presence; change that to read the backing attribute
self._system._efermi directly (i.e. compute e_fermi = self._system._efermi if
self._system._efermi is not None else 0.0) before calling exporter.write_hr so
you never invoke the property accessor that may assert.
Add support for dictionary input in ToWannier90 and ToPythTB export classes. Skip data loading when input is already a dictionary to avoid redundant processing. Update ExportAccessor to use explicit parameter names for better clarity.
…otebook - Add new_postprocess_pythtb_wannier.ipynb demonstrating PythTB and Wannier90 integration - Rename new_postprocess.ipynb to new_postprocess_band_dos.ipynb for clarity - New example includes band structure calculation with k-path configuration and Fermi level determination
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
dptb/postprocess/unified/properties/export.py (3)
46-46: Hardcodedoverlap=Falseremains unaddressed.This issue was previously flagged in past reviews. Consider exposing
overlapas a parameter to provide clearer error messaging when users attempt to export with overlap enabled.
68-68: Direct attribute access issue remains unaddressed.Line 68 was previously flagged for accessing
self._system.efermiproperty after checkingself._system._efermi. While the condition guards against None, the code should consistently useself._system._efermidirectly to avoid invoking the property accessor:-e_fermi = self._system.efermi if self._system._efermi is not None else 0.0 +e_fermi = self._system._efermi if self._system._efermi is not None else 0.0
80-80: Direct attribute access issue remains unaddressed.Line 80 has the same issue as line 68. Use
self._system._efermidirectly:-e_fermi = self._system.efermi if self._system._efermi is not None else 0.0 +e_fermi = self._system._efermi if self._system._efermi is not None else 0.0
🧹 Nitpick comments (2)
dptb/postprocess/interfaces.py (2)
37-37: Minor formatting inconsistency.Missing space after comma in
isinstance(data,dict). Line 292 has the correct formatting:isinstance(data, dict).🔎 Proposed fix
- if not isinstance(data,dict): + if not isinstance(data, dict):
31-31: Consider immutable default for AtomicData_options.While mutable defaults are a common pattern in this codebase, consider using
Noneas the default and initializing within the function to avoid potential gotchas:def _get_data_and_blocks(self, data: Union[AtomicData, ase.Atoms, dict, str], AtomicData_options: Optional[dict] = None, e_fermi: float = 0.0): if AtomicData_options is None: AtomicData_options = {} ...Also applies to: 284-284
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
dptb/postprocess/interfaces.pydptb/postprocess/unified/properties/export.pyexamples/TBSystems/new_postprocess_band_dos.ipynbexamples/TBSystems/new_postprocess_pythtb_wannier.ipynb
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: deepmodeling/DeePTB PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-26T21:32:19.567Z
Learning: Organize code into a main `dptb` package with subpackages for different functionalities (data, nn, negf, etc.)
Learnt from: CR
Repo: deepmodeling/DeePTB PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-26T21:32:19.567Z
Learning: Structure NEGF calculations in the `dptb/negf` subpackage
📚 Learning: 2025-11-26T21:32:19.567Z
Learnt from: CR
Repo: deepmodeling/DeePTB PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-26T21:32:19.567Z
Learning: Structure data loading and processing code in the `dptb/data` subpackage
Applied to files:
dptb/postprocess/interfaces.py
🧬 Code graph analysis (1)
dptb/postprocess/unified/properties/export.py (4)
dptb/postprocess/topybinding.py (2)
ToPybinding(21-95)get_lattice(42-95)dptb/postprocess/totbplas.py (2)
TBPLaS(24-183)get_cell(43-183)dptb/postprocess/interfaces.py (4)
ToWannier90(15-258)write_hr(55-174)write_win(176-214)write_centres(216-258)dptb/postprocess/unified/system.py (2)
model(68-70)data(73-75)
🪛 Ruff (0.14.10)
dptb/postprocess/interfaces.py
31-31: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
31-31: Unused method argument: e_fermi
(ARG002)
34-34: Avoid specifying long messages outside the exception class
(TRY003)
284-284: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
289-289: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
dptb/postprocess/unified/properties/export.py (1)
22-30: LGTM!The PythTB export method is correctly implemented with appropriate device propagation.
dptb/postprocess/interfaces.py (1)
284-298: LGTM!The dict support is correctly implemented and mirrors the pattern in
_get_data_and_blocks. The conditional loading preserves existing behavior while enabling pre-loaded data dictionaries to be passed directly.Note: The static analysis warning about unused
e_fermiparameter is a false positive—it's used on line 368.
Add new cells to demonstrate Wannier90 integration with PythTB, including export functionality and model loading. Update execution counts and fix reference from tbsys.band_data to tbsys.band for consistency.
Updated test assertions in test_export_unified.py to align with the actual implementation of export methods. Fixed parameter names and added missing arguments like overlap, e_fermi, and filename parameters to ensure tests accurately validate the export functionality.
Simplify the HR2HK class by removing redundant overlap-specific code paths and consolidating onsite block construction. The SOC handling is now restricted to Hamiltonian-only cases with clearer documentation about current limitations. This refactoring improves code readability and maintainability while preserving the same functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
dptb/nn/hr2hk.py (2)
57-59: Remove redundant no-op statements.Lines 58-59 have no effect:
bondwise_hoppingis already created with the correctdtypeanddeviceon line 57. Additionally,.to()and.type()return new tensors and don't modify in-place, so these calls are discarded.🔎 Proposed fix
bondwise_hopping = torch.zeros((len(orbpair_hopping), self.idp.full_basis_norb, self.idp.full_basis_norb), dtype=self.dtype, device=self.device) - bondwise_hopping.to(self.device) - bondwise_hopping.type(self.dtype)
100-118: Consider removing commented-out code.This large block of commented-out code adds noise and reduces readability. If this is preserved for reference during the transition, consider removing it once the new implementation is validated, or add a brief comment explaining why it's kept.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dptb/nn/hr2hk.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: deepmodeling/DeePTB PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-26T21:32:19.567Z
Learning: Organize code into a main `dptb` package with subpackages for different functionalities (data, nn, negf, etc.)
Learnt from: CR
Repo: deepmodeling/DeePTB PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-26T21:32:19.567Z
Learning: Structure NEGF calculations in the `dptb/negf` subpackage
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (5)
dptb/nn/hr2hk.py (5)
11-12: Good documentation clarifying the naming convention.The comment helpfully explains the discrepancy between the class name and its actual functionality, maintaining backward compatibility while documenting the intent.
66-73: SOC initialization logic looks correct.The normalization of
soctensor to boolean via.all()and conditional initialization of SOC blocks is appropriate. The comment clarifying SKTB-only support is helpful.
91-99: Verify the unified onsite block population is intentional.Line 91 now unconditionally populates
onsite_blockfor both Hamiltonian and overlap cases, whereas the commented-out code (lines 100-118) had separate handling paths. The SOC condition on line 93 correctly excludes overlap matrices per the comment.
124-128: LGTM: SOC block storage is consistent with population logic.The storage condition
soc and not self.overlapcorrectly matches the population logic on line 93. Minor note: whensoc=Trueandoverlap=True, the SOC blocks are initialized (lines 72-73) but unused - consider moving initialization inside thenot self.overlapguard if memory optimization is desired.
157-190: SOC Hamiltonian assembly looks correct.The 2N×2N SOC Hamiltonian structure is properly assembled:
- Top-left (up-up):
soc_upup_block + block- Top-right (up-down):
soc_updn_block- Bottom-left (down-up):
soc_updn_block.conj()(line 184)- Bottom-right (down-down):
soc_upup_block.conj() + blockThe overlap case correctly applies Hermitian symmetry enforcement to avoid numerical issues during Cholesky decomposition.
1 participant
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.