peteboyd / lammps_interface

automatic generation of LAMMPS input files for molecular dynamics simulations of MOFs
MIT License
125 stars 63 forks source link

maximum recursion depth exceeded #11

Closed salrodgom closed 2 years ago

salrodgom commented 5 years ago

For some MOFs with certain complexity the program crashed with this warnning "Python Maximum recursion depth exceeded". I have solve the problem incrementing the stack depth allowed with this "sys.setrecursionlimit(10000)".

ltalirz commented 2 years ago

I've just encountered this as well.

@peteboyd Do you agree that changing this setting should be done in lammps-interface? Or is it a sign of something going wrong?

peteboyd commented 2 years ago

Hi Leo, this is a sign of doing something wrong. I believe this is a result of an either breadth or depth first search for the terminus of an organic ligand where it connects to a metal ion. I would have to check to be sure, so providing me any cases you have that can repeat this error would be useful. In cases where a max recursion depth is reached, the algorithm cannot find the end of the ligand, meaning either the structure has a continuous path of organic molecules, or the code falsely assigned a bond between two organic ligands where it shouldn't have.

ltalirz commented 2 years ago

Hi pete, I see - indeed I was trying to use lammps-interface with a covalent organic framework.

Perhaps lammps-interface is anyhow less needed for those but mit might make sense to use it to have a consistent pipeline

Would it be easy to make it work there as well? E.g. one could just detect whether there are any metal atoms in the cell and skip any jobs related to those.

peteboyd commented 2 years ago

I think the more structures that can be treated with the code, the better :)

I think we could hammer out a fix for this.

ltalirz commented 2 years ago

Cheers! The exception isn't raised for every COF (although it may still be doing lots of recursions, just "flying under the radar"). Here is an example of a COF where the exception is raised:

https://raw.githubusercontent.com/danieleongari/CURATED-COFs/master/cifs/21051N2.cif

peteboyd commented 2 years ago

Cheers! The exception isn't raised for every COF (although it may still be doing lots of recursions, just "flying under the radar"). Here is an example of a COF where the exception is raised:

https://raw.githubusercontent.com/danieleongari/CURATED-COFs/master/cifs/21051N2.cif

Hey Leo, would it be possible to find a COF that doesn't raise the exception? This has me scratching my head a little.

ltalirz commented 2 years ago

hey Pete, thanks for the quick fix!

Hey Leo, would it be possible to find a COF that doesn't raise the exception?

Of course! Here is a COF that doesn't raise the exception: https://raw.githubusercontent.com/danieleongari/CURATED-COFs/master/cifs/08010N2.cif

This has me scratching my head a little.

I thought that this simply meant that for whatever reason (size of the structure, ...) the maximum recursion depth was not reached (?)

While trying other COFs I noticed that lammps-interface incorrectly recognizes a "molecule" in this COF

https://raw.githubusercontent.com/danieleongari/CURATED-COFs/master/cifs/05000N2.cif

No bonds reported in cif file - computing bonding..
totatomlen = 84
compute_topology_information()
func: cartesian_coordinates; Elps. 0.001s
func: min_img_distances; Elps. 0.034s
func: compute_bonding; Elps. 0.055s
func: init_typing; Elps. 0.065s
func: bond_typing; Elps. 0.066s
func: angles; Elps. 0.066s
func: dihedrals; Elps. 0.067s
func: improper_dihedrals; Elps. 0.067s
Molecules found in the framework, separating.
WARNING: Molecule 1 with atoms (B, O, C, B, O, C, B, O, C, B, B, B, O, O, O, C, C, H, H, C, C, H, H, C, C, H, H, C, C, H, H, C, C, H, H, C, C, H, H, C, C, C) will be using the UFF force field as no  value was set for molecules. To prevent this warning set --molecule-ff=[some force field] on the command line.
...

Perhaps it is missing parameters for boron?

SeyedMohamadMoosavi commented 2 years ago

The problem with Boron in 05000N2 is related to assigning the bonds. The distance between C-B is 1.6 A while lammps interface is assigning a bond if the pairwise distance is below tempsf(CovR_1+CovR_2) (in this case, 0.9(0.76+0.84)=1.44 A).

The issue with assigning bonding is hard to mitigate. There are always cases that we miss. A more robust way of doing it is using voronoi tessilation based approach this article, which is implemented in matminer. But it becomes very expensive for large cells.

I implemented the bonding assignment of VESTA. It is also distance based with fixed parameters (I don't know how they come up with this parameter set), but it is somewhat better than the current method in lammps interface with equal cost. I will do a PR. Unfortunately, this method does not fix the issue with Boron but helps with many other cases.