fix(dftbplus): parse all atoms instead of truncating to 4#1037
fix(dftbplus): parse all atoms instead of truncating to 4#1037abhi-0203 wants to merge 2 commits into
Conversation
The DFTB+ parser hardcoded flag checks to only read 4 coordinate rows (flag in 3,4,5,6) and 4 force rows (flag in 8,9,10,11). This silently truncated any system with more than 4 atoms. Fix: read the atom count from the GenFormat header line and use it to determine how many coordinate/force rows to parse. For forces, parse all consecutive rows after 'Total Forces' until a non-force line is encountered. Closes deepmodeling#991
dosubot
Bot
added
size:M
for more information, see https://pre-commit.ci
📝 WalkthroughWalkthroughChangesDFTB+ output parsing now reads geometry rows from the declared atom count and detects force rows dynamically. A six-atom input fixture and unit test verify symbols, coordinates, forces, shapes, and energy. DFTB+ parser handling
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 2
🧹 Nitpick comments (1)
dpdata/formats/dftbplus/output.py (1)
67-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused
n_forcesvariable.The variable
n_forcesis initialized and incremented, but its value is never used. You can safely remove it to reduce clutter.♻️ Proposed refactor
with open_file(fn_2) as f: flag = 0 - n_forces = 0 for line in f: if line.startswith("Total Forces"): flag = 8 forces = [] - n_forces = 0 elif flag == 8: # First line after "Total Forces" header contains force data # We don't know the count yet, so parse until we hit a non-force line s = line.split() if len(s) >= 4: try: # Try to parse as force row: "index fx fy fz" int(s[0]) float(s[1]) forces.append([float(s[1]), float(s[2]), float(s[3])]) - n_forces += 1 except (ValueError, IndexError): # Not a force row, we're done flag = 0🤖 Prompt for 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. In `@dpdata/formats/dftbplus/output.py` around lines 67 - 89, Remove the unused n_forces variable from the force-parsing logic, including its initialization and increment, while leaving the forces collection and parsing behavior unchanged.
🤖 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 `@tests/test_dftbplus_6atoms.py`:
- Around line 11-13: Update the sys.path insertion in
tests/test_dftbplus_6atoms.py to add the project’s parent/root directory by
traversing one level above the directory containing __file__, so the subsequent
read_dftb_plus import works during manual execution; alternatively remove the
path manipulation if the test runner already guarantees the project root on
sys.path.
- Around line 19-24: Update test_six_atoms_parsed_correctly to construct both
DFTB+ fixture paths relative to the test module’s __file__ location instead of
relying on the current working directory; preserve the existing read_dftb_plus
arguments and test behavior.
---
Nitpick comments:
In `@dpdata/formats/dftbplus/output.py`:
- Around line 67-89: Remove the unused n_forces variable from the force-parsing
logic, including its initialization and increment, while leaving the forces
collection and parsing behavior unchanged.
🪄 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: 42acbcb5-baf5-48b4-b9f6-50055595e1c6
⛔ Files ignored due to path filters (1)
tests/dftbplus/detailed_6atoms.outis excluded by!**/*.out
📒 Files selected for processing (3)
dpdata/formats/dftbplus/output.pytests/dftbplus/dftb_pin_6atoms.hsdtests/test_dftbplus_6atoms.py
| # Add parent directory to path for imports | ||
| sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) | ||
| from dpdata.formats.dftbplus.output import read_dftb_plus |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix sys.path insertion to correctly point to the parent directory.
The comment states # Add parent directory to path, but os.path.dirname(os.path.abspath(__file__)) points to the tests directory, not its parent. If the project root needs to be added to sys.path for manual test execution, it should traverse one level higher.
Alternatively, if you run your tests exclusively via a runner like pytest from the root directory, this path manipulation can be removed entirely.
🛠️ Proposed fix
# Add parent directory to path for imports
-sys.path.insert(0, os.path.dirname(os.path.abspath(__file__)))
+sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
from dpdata.formats.dftbplus.output import read_dftb_plus📝 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.
| # Add parent directory to path for imports | |
| sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) | |
| from dpdata.formats.dftbplus.output import read_dftb_plus | |
| # Add parent directory to path for imports | |
| sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) | |
| from dpdata.formats.dftbplus.output import read_dftb_plus |
🤖 Prompt for 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.
In `@tests/test_dftbplus_6atoms.py` around lines 11 - 13, Update the sys.path
insertion in tests/test_dftbplus_6atoms.py to add the project’s parent/root
directory by traversing one level above the directory containing __file__, so
the subsequent read_dftb_plus import works during manual execution;
alternatively remove the path manipulation if the test runner already guarantees
the project root on sys.path.
| def test_six_atoms_parsed_correctly(self): | ||
| """Verify that all 6 atoms are parsed, not truncated to 4.""" | ||
| symbols, coord, energy, forces = read_dftb_plus( | ||
| "dftbplus/dftb_pin_6atoms.hsd", | ||
| "dftbplus/detailed_6atoms.out", | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use robust path construction for test data files.
Hardcoded relative paths like "dftbplus/..." will fail if the test is executed from a different working directory (e.g., if pytest is run from the project root, it will look for /dftbplus in the root rather than in /tests). Constructing absolute paths relative to __file__ ensures the test passes regardless of the execution directory.
🛠️ Proposed fix
def test_six_atoms_parsed_correctly(self):
"""Verify that all 6 atoms are parsed, not truncated to 4."""
+ base_dir = os.path.dirname(os.path.abspath(__file__))
symbols, coord, energy, forces = read_dftb_plus(
- "dftbplus/dftb_pin_6atoms.hsd",
- "dftbplus/detailed_6atoms.out",
+ os.path.join(base_dir, "dftbplus", "dftb_pin_6atoms.hsd"),
+ os.path.join(base_dir, "dftbplus", "detailed_6atoms.out"),
)📝 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.
| def test_six_atoms_parsed_correctly(self): | |
| """Verify that all 6 atoms are parsed, not truncated to 4.""" | |
| symbols, coord, energy, forces = read_dftb_plus( | |
| "dftbplus/dftb_pin_6atoms.hsd", | |
| "dftbplus/detailed_6atoms.out", | |
| ) | |
| def test_six_atoms_parsed_correctly(self): | |
| """Verify that all 6 atoms are parsed, not truncated to 4.""" | |
| base_dir = os.path.dirname(os.path.abspath(__file__)) | |
| symbols, coord, energy, forces = read_dftb_plus( | |
| os.path.join(base_dir, "dftbplus", "dftb_pin_6atoms.hsd"), | |
| os.path.join(base_dir, "dftbplus", "detailed_6atoms.out"), | |
| ) |
🤖 Prompt for 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.
In `@tests/test_dftbplus_6atoms.py` around lines 19 - 24, Update
test_six_atoms_parsed_correctly to construct both DFTB+ fixture paths relative
to the test module’s __file__ location instead of relying on the current working
directory; preserve the existing read_dftb_plus arguments and test behavior.
Merging this PR will not alter performance
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1037 +/- ##
==========================================
- Coverage 87.01% 87.00% -0.01%
==========================================
Files 90 90
Lines 8330 8341 +11
==========================================
+ Hits 7248 7257 +9
- Misses 1082 1084 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Summary
The DFTB+ parser hardcoded flag checks to only read 4 coordinate rows () and 4 force rows (). This silently truncated any system with more than 4 atoms.
Closes #991
Changes
Testing
Root Cause
The original code used hardcoded flag ranges ( and ) which limited parsing to exactly 4 rows regardless of the actual system size.
Summary by CodeRabbit
Bug Fixes
Tests