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

ToolkitRegistry() should initialize an empty registry #359

Closed
j-wags opened this issue Jun 16, 2019 · 2 comments · Fixed by #684
Closed

ToolkitRegistry() should initialize an empty registry #359

j-wags opened this issue Jun 16, 2019 · 2 comments · Fixed by #684

Comments

@j-wags
Copy link
Member

j-wags commented Jun 16, 2019

@jchodera, the current default constructor for ToolkitRegistry will attempt to initialize OE, Amber, and RDK toolkits. The command to initialize an empty ToolkitRegistry is ToolkitRegistry(toolkit_precedence=[]). Revisiting this months later, this is really unintuitive, and I think we should change it (ToolkitRegistry() should initialize an empty registry). Opening a new issue.

Originally posted by @j-wags in #346 (comment)

@jchodera
Copy link
Member

If the API is breaking the "principle of least surprise", then the default behavior should definitely be changed.

I originally thought the least surprising default behavior was "do the right thing automatically unless I tell you not to"---that is, "find all the toolkits that are installed and use those". I expected that, if someone wanted fine-grained control over toolkits, they would initialize with specific toolkits or an empty set and add them one at a time, since this is the minority use case.

But I clearly got the API wrong in my debugging in #346 (comment) :)

@mattwthompson
Copy link
Member

I agree that ToolkitRegistry() should give me an instance with no wrappers registered into it. I find this type of behavior quite surprising when I ran into it:

In [1]: from openforcefield.utils.toolkits import *

In [2]: reg = ToolkitRegistry()

In [3]: [reg.register_toolkit(tk) for tk in [OpenEyeToolkitWrapper, RDKitToolkitWrapper, AmberToolsToolkitWrapper]]
Out[3]: [None, None, None]

In [4]: reg.registered_toolkits
Out[4]:
[<openforcefield.utils.toolkits.OpenEyeToolkitWrapper at 0x14071f6d0>,
 <openforcefield.utils.toolkits.RDKitToolkitWrapper at 0x144349550>,
 <openforcefield.utils.toolkits.AmberToolsToolkitWrapper at 0x144349910>,
 <openforcefield.utils.toolkits.BuiltInToolkitWrapper at 0x144349a50>,
 <openforcefield.utils.toolkits.OpenEyeToolkitWrapper at 0x1443758d0>,
 <openforcefield.utils.toolkits.RDKitToolkitWrapper at 0x14527e110>,
 <openforcefield.utils.toolkits.AmberToolsToolkitWrapper at 0x14527e050>]

(this is surprising for two reasons, one being the non-empty default registry and also that .register_toolkit should probably check to see if one is already registered)

I wouldn't expect this to be a hugely problematic API break since most functionality silently uses GLOBAL_TOOLKIT_REGISTRY behind the scenes and initializing a custom ToolkitRegistry() often comes with a populated toolkit_precedence argument. Is there a compelling reason to maintain this behavior that I'm not considering?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants