search

schrodinger / coordgenlibs

Schrodinger-developed 2D Coordinate Generation
BSD 3-Clause "New" or "Revised" License
42 stars 28 forks source link

COORDGEN-262 #80

Closed ZontaNicola closed 3 years ago

ZontaNicola commented 3 years ago

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.

greglandrum commented 3 years ago

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.

ZontaNicola commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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