fix(postprocess): normalize unified eigen solver options#334
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR extends the eigenvalue solver interface to accept configuration parameters beyond atomic data, implements centralized solver kwargs extraction with alias support in TBSystem, and adds intelligent caching to avoid recomputation when only transient parameters like ChangesSolver kwargs and eigenvalue parameter support throughout postprocessing stack
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
1fda6fe to
5715328
Compare
5715328 to
c7dfeb1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dptb/postprocess/unified/properties/dos.py`:
- Around line 175-179: PDOS mode currently logs ignored options for solver and
ill_threshold but not for ill_pad_value; update the PDOS path where data, eigs,
vecs = self._system.calculator.get_eigenstates(...) is called to also emit a
warning when ill_pad_value is not None (e.g., log.warning("ill_pad_value is
ignored for PDOS because eigenvector padding is not implemented.")) so callers
are informed; locate the block that checks solver and ill_threshold and add the
analogous check for ill_pad_value.
In `@dptb/postprocess/unified/system.py`:
- Around line 127-139: The bug is that _dos_cache_key omits the "dos_config"
entry when kmesh is None, causing different DOS requests with kmesh=None but
different erange/npts/etc. to collide; fix _dos_cache_key (the class method that
calls cls._eigen_cache_key and cls._normalize_cache_value) to always include a
normalized dos_config in the returned cache_key (use the full dict with "kmesh":
kmesh, "is_gamma_center": is_gamma_center, "erange": erange, "npts": npts,
"smearing": smearing, "sigma": sigma, "pdos": pdos, "extra_kwargs": extra_kwargs
or {}) so None is preserved and changes to other parameters prevent stale cache
hits.
🪄 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: 334a85b1-9394-41ed-a155-f21c61b5422b
📒 Files selected for processing (5)
dptb/postprocess/unified/calculator.pydptb/postprocess/unified/properties/band.pydptb/postprocess/unified/properties/dos.pydptb/postprocess/unified/system.pydptb/tests/test_postprocess_unified.py
This PR extracts the still-useful part of #325 into a smaller change based on the current
mainbranch.Fixes #324.
Summary
eig_solveras a compatibility alias in unifiedTBSystemAPIs.solveras the preferred unified API name.ValueErrorif bothsolverandeig_solverare provided with different values.nkthrough unified band and DOS eigenvalue calculations so users can control k-point chunking.nkout of cache invalidation because it only controls k-point chunking and does not change the sampled k-points or numerical result.Notes
The legacy APIs and run configs still use
eig_solver; the unifiedTBSystemlayer now accepts it for migration compatibility and normalizes it internally tosolverbefore calling the lower-level eigenvalue code.This intentionally does not include the
override_overlapchanges from #325. Currentmainalready has per-calloverride_overlapsupport with newer overlap/eigensolver handling, while #325's instance-level default/opt-out approach adds extra state semantics and should not be carried into this focused fix.Validation
python -m py_compile dptb/postprocess/unified/system.py dptb/postprocess/unified/properties/band.py dptb/postprocess/unified/properties/dos.py dptb/tests/test_postprocess_unified.pyuv run pytest dptb/tests/test_postprocess_unified.py -quv run pytest dptb/tests/test_band.py -q