Closes #1536: implement periodic boundary conditions #1853

bbucior · 2018-06-09T13:29:48Z

This PR partially implements periodic boundary conditions in Open Babel by applying the minimum image convention and using OBUnitCell data. Some formats or existing code may not be expecting this behavior, so it is currently hidden behind an OB_PERIODIC_MOL flag in OBMol. See also openbabel/enhancement-proposals#6 for more discussion. Currently this PR is more of a minimum viable product, but over the summer I'm planning to submit additional PR's after combing through more of the codebase.

This PR also adds an option to write bonding information to CIF output, including notation for bonds across periodic boundaries.

Before the code can be merged, there's a handful of details to take care of:

There are some merge conflicts (and we should be careful that any changes are compatible with PR#1823). What's the quickest/cleanest way to address these?
Const correctness is tricky: I couldn't keep the const keyword on OBBond::GetLength() without making a lot of other changes to OBMol::HasFlag and some related methods.
What is best practice for the argument of OBGenericData->Clone(NULL)? Is it NULL, the OBMol, or something context-dependent?
Any other comments or cleanup would also be appreciated. Thanks!

Special thanks to Giovanni Garberoglio for helpful discussion and his code in http://software-lisc.fbk.eu/obgmx/. I tried to combine the best parts of both of our PBC implementations in this PR.

Used `git apply` to apply relevant code for periodic boundary conditions from an ongoing research project. More cleanup necessary before upstream integration.

Adopt MinimumImageFractional and MinimumImageCartesian notation from Giovanni Garberoglio's implementation to replace the PBCDifference methods.

Avoid cluttering the Open Babel API, now that earlier research code has been refactored.

My previous iteration of periodic boundary conditions in OBMol added an extra copy of OBUnitCell* to the OBMol object, which was undesirable because (1) it clutters the API, and (2) leads to consistency problems. Now, there is only one copy of OBUnitCell* within the molecule's data.

These cases cover periodic boundaries and bond output for CIFs

include/openbabel/mol.h

ghutchis · 2018-06-11T17:49:34Z

Overall, this looks good. Obviously having a periodic UFF would be great.

The only way to handle merge conflicts is manually. So take mmcifformat.cpp from master and work in any remaining changes, etc.
OBBond::GetLength really needs to be const
The Clone command is called when copying generic data from one molecule to another. If you're keeping track of atom pointers, etc. you'll need a context-dependent version..

Periodic boundary code is based on an old version of Open Babel, so I attempted to manually resolve the merge conflicts identified by Github (and ran diff against the current master and periodic-boundaries branches to verify).

Updated mmcifformat.cpp, mol.cpp, and CMakeLists.txt to resolve remaining merge conflicts.

bbucior · 2018-06-15T02:39:01Z

Thank you for the feedback.

Conflicts resolved
One challenge I ran into was that some of the existing code wasn't declared as const. I can restore const to OBBond::GetLength if some of the other methods are declared as const. Before making a commit, does this example change look reasonable, or are there other concerns about API compatibility, etc?
I think I understand. I don't recall anything seeing anything context-specific in OBUnitCell, but keeping track of the molecule anyway probably wouldn't hurt. The line I was looking at is newmol.SetData(parent_uc->Clone(NULL)); (in OBMol::CopySubstructure), so I think the relevant OBMol would be &newmol.

Making sure the UFF code is PBC-compatible sounds like a great feature. I will work on that after porting some more PBC features from my research code (such as different formats than CIF) to another PR.

ghutchis · 2018-07-18T21:49:27Z

Okay, this looks okay if you can fix the new set of conflicts. Thanks for your patience - I think this will be really helpful. (I have some ideas about packing molecules using code like this and distance geometry / fragment placement.)

Resolved remaining merge conflicts in mol.h and mol.cpp

Restored const to OBBond::GetLength. Avoided changes to existing API by dereferencing the `this` pointer as done by similar methods in the code.

bbucior · 2018-07-22T13:01:15Z

Great! I resolved the merge conflicts and made sure not to change any of the const declarations in the existing API. Thanks for your help--I think this is good to go.

bbucior · 2018-09-05T15:31:56Z

@ghutchis I'm going to try and get the periodic UFF code ready as a separate PR next week. Do you think you'll have time to review this PR before then?

The coordinates in the atom and bond fields did not match because wrapping had been applied inconsistently. This discrepancy confused a visualization program when parsing the bond symmetry flags.

bbucior · 2019-03-12T02:27:20Z

I cleaned up the new merge conflicts and patched a bug I had found while using the code in my research. Those are all the changes I had planned for now, unless there are any requests.

ghutchis · 2019-09-14T19:20:44Z

I'm going to commit this to master - hopefully the merge was successful. (We'll find out once it re-runs tests.)

# Resolved Conflicts: # test/CMakeLists.txt

bbucior · 2020-01-25T20:53:18Z

The build is passing again now that merge conflicts have been resolved and the branch has been updated in light of #1951 and #1958.

ghutchis · 2020-01-25T21:15:15Z

Looks good - I'm going to merge this. Many thanks!

baoilleach · 2020-05-03T06:44:15Z

Just looking at this now in the context of the 3.0.1 release. I see a couple of TODOs in there based on memory allocation and pointers. Do you need/want feedback on these?

bbucior · 2020-05-09T11:37:09Z

Hi @baoilleach hope you are doing well. Thanks for your interest and offer to help. Is there any section of code in particular that would be good to clean up? From what I remember, most of the remaining TODOs were either in SetLength (and related methods) or file formatters that still calculated geometry from raw vector3's instead of atom/bond distances.

I'd be happy to take another look and do some more cleanup, though making Open Babel fully PBC-aware wouldn't be something I could commit to right now. I'm not currently dogfooding the code for research, and implementing PBC the right way might require some larger architectural discussions to see if there's a way to more generically handle coordinates (perhaps an OBCoord class?).

baoilleach · 2020-05-10T17:14:01Z

Currently tied up with the release. Let's talk later...

bbucior added 5 commits September 1, 2017 16:06

Implement periodic boundary conditions

68abae0

Used `git apply` to apply relevant code for periodic boundary conditions from an ongoing research project. More cleanup necessary before upstream integration.

Clean up API for minimum image convention

5aae885

Adopt MinimumImageFractional and MinimumImageCartesian notation from Giovanni Garberoglio's implementation to replace the PBCDifference methods.

Remove extra OBBond::GetPeriodicDirection method

d76d9f2

Avoid cluttering the Open Babel API, now that earlier research code has been refactored.

Write unit tests

aa8c81a

These cases cover periodic boundaries and bond output for CIFs

ghutchis requested changes Jun 11, 2018

View reviewed changes

include/openbabel/mol.h Outdated Show resolved Hide resolved

include/openbabel/mol.h Outdated Show resolved Hide resolved

bbucior added 2 commits June 14, 2018 21:08

Attempt to manually resolve merge conflicts for PBC

4b34299

Periodic boundary code is based on an old version of Open Babel, so I attempted to manually resolve the merge conflicts identified by Github (and ran diff against the current master and periodic-boundaries branches to verify).

Merge branch 'origin/master' into periodic-boundaries

3212f6f

Updated mmcifformat.cpp, mol.cpp, and CMakeLists.txt to resolve remaining merge conflicts.

bbucior added 2 commits July 21, 2018 16:42

Merge branch 'origin/master' into periodic-boundaries

4cc2543

Resolved remaining merge conflicts in mol.h and mol.cpp

Avoid changing const of OBBond::GetLength

45b5777

Restored const to OBBond::GetLength. Avoided changes to existing API by dereferencing the `this` pointer as done by similar methods in the code.

Merge branch 'master' into periodic-boundaries

fa505c4

bbucior mentioned this pull request Mar 6, 2019

Update Open Babel snurr-group/mofid#13

Open

bbucior added 2 commits March 11, 2019 20:32

Merge branch 'master' into periodic-boundaries

7319dba

Fix bug with writing periodic bonds to CIF

a46f12c

The coordinates in the atom and bond fields did not match because wrapping had been applied inconsistently. This discrepancy confused a visualization program when parsing the bond symmetry flags.

ghutchis approved these changes Mar 12, 2019

View reviewed changes

Merge branch 'master' into periodic-boundaries

805cbd5

ghutchis and others added 6 commits December 4, 2019 22:05

Merge branch 'master' into periodic-boundaries

25c5a71

Merge branch 'master' into periodic-boundaries

27e0817

# Resolved Conflicts: # test/CMakeLists.txt

Apply conventions from openbabel#1951 to SetPeriodicMol

e4c0d4a

Include OBUnitCell symbol for atom/bond periodicity

d84b3c3

Include OBUnitCell in stereo perception

76e77a6

Specify includes for the periodicity tests

884b164

ghutchis merged commit 3d6384b into openbabel:master Jan 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closes #1536: implement periodic boundary conditions #1853

Closes #1536: implement periodic boundary conditions #1853

bbucior commented Jun 9, 2018

ghutchis commented Jun 11, 2018

bbucior commented Jun 15, 2018

ghutchis commented Jul 18, 2018

bbucior commented Jul 22, 2018

bbucior commented Sep 5, 2018

bbucior commented Mar 12, 2019

ghutchis commented Sep 14, 2019

bbucior commented Jan 25, 2020

ghutchis commented Jan 25, 2020

baoilleach commented May 3, 2020

bbucior commented May 9, 2020

baoilleach commented May 10, 2020

Closes #1536: implement periodic boundary conditions #1853

Closes #1536: implement periodic boundary conditions #1853

Conversation

bbucior commented Jun 9, 2018

ghutchis commented Jun 11, 2018

bbucior commented Jun 15, 2018

ghutchis commented Jul 18, 2018

bbucior commented Jul 22, 2018

bbucior commented Sep 5, 2018

bbucior commented Mar 12, 2019

ghutchis commented Sep 14, 2019

bbucior commented Jan 25, 2020

ghutchis commented Jan 25, 2020

baoilleach commented May 3, 2020

bbucior commented May 9, 2020

baoilleach commented May 10, 2020