Add FCHK connectivity (bonds) parsing support #400
Conversation
- Add 'bonds' to @document_load_one optional attributes - Add MxBond, NBond, IBond, RBond label patterns - Add _load_connectivity() to parse bond data - Add _dump_connectivity() to write bond data - Add 6 tests for connectivity loading/dumping Fixes theochem#399
There was a problem hiding this comment.
Pull request overview
This PR adds support for parsing and writing bond connectivity information in Gaussian FCHK files. The implementation follows OpenBabel's approach, using the MxBond, NBond, IBond, and RBond fields from the FCHK format to store bond connectivity and bond orders. Bonds are stored uniquely by only keeping entries where atom_i < atom_j, avoiding duplicates since each bond appears twice in the FCHK format (once from each atom's perspective).
Changes:
- Added connectivity parsing in
_load_connectivity()function with robust error handling for missing or invalid data - Added connectivity writing in
_dump_connectivity()function that properly expands bonds bidirectionally - Added comprehensive tests covering various molecules (H2O, H2O2, Li2) and edge cases (files with zero bonds)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| iodata/formats/fchk.py | Implements bond connectivity parsing and dumping with helper functions _load_connectivity and _dump_connectivity; integrates bond support into load_one and dump_one functions |
| iodata/test/test_fchk.py | Adds 6 new test functions covering bond loading from various FCHK files, edge cases with zero bonds, and roundtrip dump/load preservation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
FanwangM
left a comment
FanwangM
left a comment
There was a problem hiding this comment.
Thanks for helping with this feature! Just some minor comments. @aamrindersingh
|
@FanwangM done with vectorized both functions and switched to raising errors. lmk if anything else needs work |
|
I am OK with the current shape. Do you have any comments or suggestions? Thanks. @tovrstra |
|
I am going to merge it. The failing test is Sphinx-related. |
2 participants
Fixes #399
"Approach"
The FCHK format stores bond info in four fields: MxBond, NBond, IBond, and RBond. I followed the approach from OpenBabel's implementation (https://github.com/openbabel/openbabel/blob/889c350feb179b43aa43985799910149d4eaa2bc/src/formats/fchkformat.cpp#L651-L690) and adapted it for IOData.
The format stores each bond from both atoms' perspectives (so an O-H bond appears twice — once for O, once for H). To get a clean bond list, I only keep the entry where atom_i < atom_j. This is the same approach OpenBabel uses.
"Tests"
I used files already in the repo that have connectivity data: h2o_sto3g.fchk, peroxide_opt.fchk, li2_g09_nbasis_indep.fchk, and water_atcharges.fchk (which has the fields but no actual bonds). Also added roundtrip tests to make sure dump and load preserve everything correctly. All 6 new tests pass.