-
Notifications
You must be signed in to change notification settings - Fork 22
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
PR to verify disorders #63
Conversation
…ixes ccdc-opensource#58 but improves it somewhat. There doesn't seem to be any consistent ways to check for disorder
…are no 3d coordinates and gives out a warning.
if io.CrystalReader('CSD').entry(structure).has_disorder and not force_run: | ||
raise RuntimeError("Disorder can cause undefined behaviour. It is not advisable to run this " | ||
"script on disordered entries.\n To force this script to run on disordered entries" | ||
" use the flag --force_run_disordered.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are working on better disorder handling for structures already in the CSD so when that project is done this might need to be edited as it will be possible to run on one of the assemblies of the disordered molecule
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I looked into it and could not find on the documentation a way to do that. But if it is on the works that's nice. Will it automatically do it? Because if the script will have to be modified anyway after the fact, I don't see the problem in using this as a bandaid. Furthermore, someone could go on Mercury and create a mol2 without disorder for running. So I think this is a good solution for now. As having a warning is better than having weird errors pop up later. If it needs to be changed, it is trivial to get the file before this changes from git history.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already the option to do this for CIFs, with crystal.disordered_molecule, but not refcodes yet. It won't be automatic so it will still need a new edit but I agree that for now this works. Was just letting you know that at some point the handling of disorder will be better :)
failure_warning = f"Could not run for {coformer_name} no 3d coordinates present." | ||
failures.append(failure_warning) | ||
warnings.warn(failure_warning) | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not too sure about this one. We sometimes use the scripts on 2D structures when a crystal structure is not available. I see that it still runs but maybe we can have a flag to skip the warning and appending it to the error list. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will take a look. The main problem seems to be that I think it would be nice to have some file validation. I will close this PR, refactor a little my git history to make it so the improvements to error handling are in a separate commit and work on creating a better validation for files.
I stupidly forgot to verify if all mol2 files had 3d coordinates when creating a database of API and GRAS entries for my lab. I thought it would be a nice warning to have on the run. Furthermore, verifying if entries that use refcodes have disorder is trivial and I did it in the first commit. As this script uses a library of structural files, each file type handles disorder in a different way. So, it would be difficult to verify if the structure is disordered.
Also, a while ago I thought it would be much better to provide the error message on the try-except statement. I totally forgot of committing it and including it in a PR, as I am using a fork of this script for my purposes. I am sorry for that. I had the impression I had included the improvement in the way errors are handled in an older PR.