Skip to content

COORDGEN-262#80

Merged
d-b-w merged 4 commits intoschrodinger:masterfrom
ZontaNicola:master
Mar 6, 2021
Merged

COORDGEN-262#80
d-b-w merged 4 commits intoschrodinger:masterfrom
ZontaNicola:master

Conversation

@ZontaNicola
Copy link
Copy Markdown
Collaborator

Always trust bond order for terminal metals (in other words never convert bonds to terminal atoms to zero order bonds), this means that terminal atoms are treated as part of the same molecule for depiction purposes, so the bond is always a standard length.

Copy link
Copy Markdown
Collaborator

@d-b-w d-b-w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test for this.

Comment thread sketcherMinimizer.cpp
@greglandrum
Copy link
Copy Markdown
Collaborator

I think having this option makes sense, but it does make the name of the option quite misleading - functionally it would no longer be TreatAllBondsToMetalAsZOBs.

I'm not sure how stable you need to keep the API, particularly with this new function, but ideally that option name could be changed to TreatNonterminalBondsToMetalAsZOBs or something equivalent. At the very least there should be some documentation for it.

add test, rename option
@ZontaNicola
Copy link
Copy Markdown
Collaborator Author

I think having this option makes sense, but it does make the name of the option quite misleading - functionally it would no longer be TreatAllBondsToMetalAsZOBs.

I'm not sure how stable you need to keep the API, particularly with this new function, but ideally that option name could be changed to TreatNonterminalBondsToMetalAsZOBs or something equivalent. At the very least there should be some documentation for it.

I think RDKit is the only place where this is used, if you're ok with updating the name, I'd just do that

@greglandrum
Copy link
Copy Markdown
Collaborator

I think having this option makes sense, but it does make the name of the option quite misleading - functionally it would no longer be TreatAllBondsToMetalAsZOBs.
I'm not sure how stable you need to keep the API, particularly with this new function, but ideally that option name could be changed to TreatNonterminalBondsToMetalAsZOBs or something equivalent. At the very least there should be some documentation for it.

I think RDKit is the only place where this is used, if you're ok with updating the name, I'd just do that

No problem on the RDKit side. We haven't done a release that includes that functionality yet anyway (it's still on master).

@d-b-w
Copy link
Copy Markdown
Collaborator

d-b-w commented Mar 4, 2021

I see that you added a test, but I don't understand how it validates the new feature, since setTreatNonterminalBondsToMetalAsZOBs() is turned off in the test. Maybe the test should exercise both states?

Since this is only about terminal bonds, we don't expect this to help with https://jira.schrodinger.com/browse/CRDGEN-264, right?

@ZontaNicola
Copy link
Copy Markdown
Collaborator Author

I see that you added a test, but I don't understand how it validates the new feature, since setTreatNonterminalBondsToMetalAsZOBs() is turned off in the test. Maybe the test should exercise both states?

Since this is only about terminal bonds, we don't expect this to help with https://jira.schrodinger.com/browse/CRDGEN-264, right?

so now the treatment of non-terminal metals is controlled by the flag, while for terminal metals it's "always on", so the test for terminal metals doesn't need the flag to be set.
correct, this won't help with CRDGEN-264, nor does the setting/unsetting of the flag.

@d-b-w d-b-w merged commit 56a4285 into schrodinger:master Mar 6, 2021
@d-b-w
Copy link
Copy Markdown
Collaborator

d-b-w commented Mar 19, 2021

This change will require a version bump, because it changes the API of the library

d-b-w added a commit that referenced this pull request Mar 24, 2021
For use in Schrödinger release 2021-1.

Set version in CMakelists to 2.0.0. There was a change in an option name (TreatAllBondsToMetalAsZOBs became TreatNonterminalBondsToMetalAsZOBs #80). Otherwise, this is mostly a bugfix release.
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

Successfully merging this pull request may close these issues.

3 participants