openff-toolkit icon indicating copy to clipboard operation
openff-toolkit copied to clipboard

`ForceField` `sources` allows strings but description doesn't seem mention it.

Open IAlibay opened this issue 9 months ago • 2 comments

Overview

Looking at the definition for sources, my understanding is that the docstring always seems to indicate that a file or file-like object is required. This can be seen in either in __init__: https://github.com/openforcefield/openff-toolkit/blob/9934858211cd3554f855d18aa40af730218fa1a0/openff/toolkit/typing/engines/smirnoff/forcefield.py#L241-L248 or in parse_sources: https://github.com/openforcefield/openff-toolkit/blob/9934858211cd3554f855d18aa40af730218fa1a0/openff/toolkit/typing/engines/smirnoff/forcefield.py#L772-L779 or in parse_smirnoff_from_source: https://github.com/openforcefield/openff-toolkit/blob/9934858211cd3554f855d18aa40af730218fa1a0/openff/toolkit/typing/engines/smirnoff/forcefield.py#L985-L989

However, looking a bit further below, it looks like we can indeed parse an FFXML as a string directly: https://github.com/openforcefield/openff-toolkit/blob/9934858211cd3554f855d18aa40af730218fa1a0/openff/toolkit/typing/engines/smirnoff/forcefield.py#L1038

The question

Is this intended? Is string parsing officially supported but the docstring is out of date, or is this something that is more private API-esque?

The use case

I want to be able to allow folks to define OFFXML strings when creating an OpenFE FF settings object and not having to wrap it into a file-like object after the fact would make life a lot easier.

IAlibay avatar Feb 12 '25 00:02 IAlibay

As a passer-by, that docstring could use a heavy dose of whitespace 😅

Is this intended?

Hard to ascribe intent, but this is something that's been in the API/functionality since the beginning and is heavily baked into the test suite (look at several hundred lines in openff/toolkit/_tests/test_forcefield.py). I can't imagine it going away soon

mattwthompson avatar Feb 12 '25 03:02 mattwthompson

Yeah, at this point I think the docstring is wrong. I consider the ability of this methods to load OFFXML strings to be de facto in the public API so in my mind this a stable feature with deficient documentation. Feel free to continue using it this way.

j-wags avatar Feb 12 '25 17:02 j-wags