Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve CIF parser to accept symmetric MOFs. #5

Open
kbsezginel opened this issue Sep 26, 2017 · 7 comments
Open

Improve CIF parser to accept symmetric MOFs. #5

kbsezginel opened this issue Sep 26, 2017 · 7 comments

Comments

@kbsezginel
Copy link
Contributor

I think the CIF parser can be improved to accept non-P1 MOFs. ASE Python library has a pretty good CIF parser so that can be used to read MOFs or even other crystals. Moreover, it can read many file formats, allowing other file formats to be accepted. I would be willing to work on this.

@kbsezginel kbsezginel changed the title Improve cif parser to accept symmetric MOFs. Improve CIF parser to accept symmetric MOFs. Sep 26, 2017
@tawe141
Copy link

tawe141 commented Apr 24, 2020

Has there been any work on this? This would also fix #1

@ltalirz
Copy link
Collaborator

ltalirz commented Apr 24, 2020

I don't think so - right @peteboyd ?

@ltalirz
Copy link
Collaborator

ltalirz commented May 25, 2020

This is something that would be quite simple to add, in particular once we have the first basic continuous integration tests in place (which would guard against unintended consequences of the shift)

@ltalirz
Copy link
Collaborator

ltalirz commented Jun 18, 2021

@peteboyd It turns out Henglu just stumbled upon this as well.

Would you be open to adding a dependency on ASE to get a fully fledged CIF parser?

I see that you already thought about this in

def ase_from_CIF(cifname):
'''
Try to read the cif file using the ase environment. They have considerations for space groups.
We don't.
'''
from ase.io import read
ase_cif = read(cifname)
mg = MolecularGraph(name=clean(cifname))
for atom in ase_cif:
print(atom)

Is there more to do than make ase_from_CIF return the MolecularGraph and the cell like from_CIF does?
Does lammps-interface use aspects of the CIF file other than elements and positions of the atoms?

If it's just the above, I think it would be quite straightforward to do.

@peteboyd
Copy link
Owner

The only thing that would require a little attention is when the code reads in bonding information from the CIF file. I'm not sure if that part of the code is still relevant, but it would have to be investigated before proceeding with the ASE implementation.

@SeyedMohamadMoosavi
Copy link
Collaborator

Reading the bonding information from CIF is absolutely useful when we have structures generated using one of the topology-based structure generators.
I suggest switching to ASE for the general cases and adding a new argument input when one needs to read the bonding topology from CIF (keeping the current pieces of the code).
This has an extra advantage as ASE periodic distance computation is much faster than the one in lammps interface --> it allows dealing with larger MOFs.

@peteboyd
Copy link
Owner

Reading the bonding information from CIF is absolutely useful when we have structures generated using one of the topology-based structure generators.
I suggest switching to ASE for the general cases and adding a new argument input when one needs to read the bonding topology from CIF (keeping the current pieces of the code).
This has an extra advantage as ASE periodic distance computation is much faster than the one in lammps interface --> it allows dealing with larger MOFs.

Thanks - good point about the weird bonding cases that come from crystal building programs. Bonding can be dealt with using symmetry operations, we just have to be careful about it. I recall the notation in the CIF format is a bit strange.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants