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

Critical typo in test helperFuncs.py #47

Closed
ralf-meyer opened this issue Jan 12, 2022 · 9 comments
Closed

Critical typo in test helperFuncs.py #47

ralf-meyer opened this issue Jan 12, 2022 · 9 comments

Comments

@ralf-meyer
Copy link
Member

ralf-meyer commented Jan 12, 2022

There is a minor typo on line 133 in helperFuncs.py:
pssLG = False instead of passLG = False

This causes a false negative (or positive?) test result. Fixing the typo breaks test_example_3.py. I have no idea what this test is supposed to check for. Since the original contributors are no longer here: Can anyone else help me out?

@ralf-meyer
Copy link
Member Author

ralf-meyer commented Jan 12, 2022

On a related note: test_bidentate.py and test_example_5.py both fail on my M1 Mac. Can anyone confirm this behavior?

@ralf-meyer ralf-meyer mentioned this issue Jan 13, 2022
@naveenarun
Copy link
Contributor

On a related note: test_bidentate.py and test_example_5.py both fail on my M1 Mac. Can anyone confirm this behavior?

This is expected (yet unfortunate) behavior, as the build for M1 Macs uses OpenBabel 3, which doesn't count hydrogens when indexing atoms of a ligand built from a SMILES string. For test_bidentate.py, it uses the input file bidentate_lig.smi which has hydrogen-containing ligands. The same is true for example_5.in.

For example, with example_5.in, in OpenBabel 2 (assuming indexing starts at 1), the position of n1 in the SMILES string c1(nn[nH]n1)c1ncccc1 will be 6, whereas in OpenBabel 3 it will be 5. This is an issue because the -smicat 8,6 flag specifies which indices should be considered connecting atoms. OpenBabel2 will correctly call n1 a connecting atom; OpenBabel 3 will instead choose the second c1.

@naveenarun
Copy link
Contributor

For test_example_3, I think something weird is happening regardless of whether passLG is fixed. In example_3_noff.in, it looks like molSimplify distorts ferrcore.xyz despite the fact that ffoption is set to no @chenruduan could you take a look?

image

@chenruduan
Copy link
Contributor

The reference results of test_example3 is a incorect–someone placed a .xyz file without FFopt as a reference. If the FF is on, the structure breaks apart… It may take some time to see why that is… @ralf-meyer Can you try whether the newly added xtb can fix the optimization? It seems a bit strange on the FF end.

@ralf-meyer
Copy link
Member Author

This thread has gotten somewhat out of sync due to yesterdays PR (#49). The issue mentioned by @naveenarun

molSimplify distorts ferrcore.xyz despite the fact that ffoption is set to no

has been fixed since I stumbled upon that bug more or less by coincidence.

The remaining problem is a wrong reference for example_3.xyz. As mentioned by @chenruduan force field optimization with MMFF94 breaks the structure (test case fails with wrong number of ligands / atoms per ligand):
result_mmff94

While xtb does not destroy the structure:
result_xtb
I am not sure if this solution is in the "spirit" of the test case.

What was the original intention of this test?
I guess that it is supposed to test structure generation for multi-atom cores. Which raises the question, what constraints should be applied during such an optimization? Which atoms / bond lengths should be frozen?

@adityanandy
Copy link
Contributor

Just going to chime in here with one small detail to answer Ralf's question. I'm fairly certain that this test case was there to go hand-in-hand with the custom core functionalization tutorial: http://hjkgrp.mit.edu/content/molsimplify-tutorial-3-custom-core-functionalization --> which is to ensure that the replig function works

@ralf-meyer
Copy link
Member Author

Thank you @adityanandy! I am still confused: There is a separate test case for the tutorial (test_tutorial_3.py) that is basically just a reduced version of the ffoption no version of test_example_3

@adityanandy
Copy link
Contributor

That is a really good point. I don't remember off of the top of my head why there are both of those. We had a hackathon where we were making test cases for all of the tutorials, so we probably just didnt realize one existed already

@ralf-meyer
Copy link
Member Author

PR #52 fixes this issue. For now test_example_3 is switched from MMFF94 to UFF as this is the simplest fix. However, this choice still leads to a breakup of the ligand, which brings us back to the question: What constraints should be applied here? Which atoms should be considered as "connecting atoms"?

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

4 participants