search

danieleongari / CURATED-COFs

Clean, Uniform and Refined with Automatic Tracking from Experimental Database (CURATED) COFs
MIT License
34 stars 11 forks source link

errors in COFs #35

Closed SimonEnsemble closed 2 years ago

SimonEnsemble commented 2 years ago

overlapping atoms in:

xtals_w_overlap = ["20010N2", "21131N2"]
image
mpougin commented 2 years ago

hello @SimonEnsemble, thank you for spotting this. @danieleongari, how do you handle these? Cleaning them manually?

danieleongari commented 2 years ago

Thanks @SimonEnsemble for the prompt notice.

@mpougin, first it would be nice to understand why our hooks on the commits did not spot this. Second, I would suggest to take the usual pragmatic approach: if it takes more than 15-30 minutes to fix each of them and they are not coming from a very-high impact journal, you can move them to the list of "discarded" CIFs and that's it. Thanks for handling this!

mpougin commented 2 years ago

thanks for your feedback @danieleongari. I will come back to it by middle/end of the week

SimonEnsemble commented 2 years ago

our Xtals.jl code detects overlap automatically; this is how we found the error. when we try to load in the .cif, it fails.

CC @eahenle 

using Xtals
xtal = Crystal("cif_name.cif", check_overlap=true) # well, check_overlap is by default true.
eahenle commented 2 years ago

There is also an "overlap" between atoms in 21192N2.cif, because of periodic boundary conditions--there are two images of the structure, each of which lies in a parallel boundary plane of the unit cell (making them actually coincident).

Inspecting the structure shows another artifact: "orphan" hydrogens hanging out in the middle of empty space. image

There are 64 additional structures that raise warnings on calling Xtals.infer_bonds!, which may indicate this kind of lone-atom artifact occurs elsewhere. I will look into this and report back if I find additional issues.

eahenle commented 2 years ago

There are a few problems in the structures I reviewed. See my PR for details. Happy to help you reproduce the workflow, or possibly incorporate an Xtals.jl workflow into your CI to help detect these kinds of things in the future.

eahenle commented 2 years ago

I think @mpougin and I have sorted this all out. Once #36 is merged, this issue can close.

mpougin commented 2 years ago

Thank you for your valuable input @SimonEnsemble and @eahenle, I close this now