Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates dependencies (tblite to main/HEAD, multicharge to v0.5.0, mctc-lib to v0.5.1, and dftd4 to v4.0.1) to make xtb compatible with the latest versions. The key changes include adding a dedicated PTB solver to replace removed tblite functionality and implementing HOMO index finding logic since it's no longer provided by the tblite wavefunction.
- Adds new PTB solver module to handle eigenvalue problems with custom orbital energy scaling
- Updates EEQ model initialization to use new_eeq_model API with error handling
- Implements HOMO index calculation based on electron count and fractional occupations
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 29 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ptb/solver.F90 | New PTB solver implementation extending tblite's sygvd_solver with custom orbital energy scaling |
| src/ptb/twostepscf.F90 | Replaces old get_density function with new PTB solver usage, updates EEQ model solve API |
| src/ptb/response.F90 | Updates to use new PTB solver for polarizability calculations |
| src/ptb/calculator.F90 | Updates EEQ model initialization to new API with error handling |
| src/ptb/ncoord.F90 | Changes import from tblite_data_covrad to mctc_data |
| src/tblite/mapping.F90 | Implements HOMO index calculation to replace removed tblite%homo field |
| src/tblite/calculator.F90 | Adds error parameter to eeq_guess and imports new eeqbc_guess |
| src/dipro.F90 | Implements HOMO index calculation for fragment analysis |
| test/unit/test_ptb.F90 | Updates test to use new EEQ model API with error handling |
| subprojects/*.wrap | Updates dependency versions for tblite, multicharge, mctc-lib, and dftd4 |
| cmake/modules/Find*.cmake | Adds/updates CMake modules for multicharge and dftd4, updates tblite and mctc-lib versions |
| src/ptb/meson.build | Adds solver.F90 to build |
| src/ptb/CMakeLists.txt | Adds solver.F90 to build |
Comments suppressed due to low confidence (2)
subprojects/tblite.wrap:4
- The revision is set to 'head' which tracks the latest commit on the main branch. This can lead to non-reproducible builds and unexpected breaking changes. The PR description mentions this is intentional until tblite 0.5.1 is released, but consider pinning to a specific commit hash in the interim to ensure build reproducibility.
revision = head
cmake/modules/Findtblite.cmake:20
- The revision is set to 'HEAD' (uppercase) which tracks the latest commit. This creates inconsistency with the meson build file which uses 'head' (lowercase), and can lead to non-reproducible builds. Consider using a specific commit hash or version tag, and ensure consistency between build systems.
set(_rev "HEAD")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
#1295 can be also fixed by this PR |
|
Let's update the dependencies to add new features with tblite (solvation and electric fields), even though we don't have a new tblite release. For a release, we should pin the tblite dependency again. |
chselz
left a comment
There was a problem hiding this comment.
Looks good, just minor comments
Update for the tblite, DFT-D4, and multicharge dependencies. This PR updates the tblite dependency to the current main branch of tblite and is a draft until the 0.5.1 tblite release (tblite#305).
To make xtb compatible with the updated dependencies, I added a dedicated PTB solver and searched for the HOMO index (no longer in the tblite wavefunction). Additionally, I added consistent dependencies in meson and cmake.