Overhaul protomer enumeration

openforcefield / openff-toolkit

The Open Forcefield Toolkit provides implementations of the SMIRNOFF format, parameterization engine, and other tools. Documentation available at http://open-forcefield-toolkit.readthedocs.io

http://openforcefield.org

MIT License

302 stars 88 forks source link

Overhaul protomer enumeration #1779

Closed mattwthompson closed 5 months ago

mattwthompson commented 7 months ago

Suggestion #1 in https://github.com/openforcefield/openff-toolkit/issues/1464#issuecomment-1328660311

[x] Tag issue being addressed
[x] Add tests
- [x] Test a molecule which is itself not a strong protomer is not returned
- [x] Test a molecule which is a sensible protomer is returned
- [x] Test default max_states finds as many states as it can
- [x] Test too-large max_states just returns whatever OpenEye finds
- [x] Test that 0 < max_states < len(openeye_protomers) returns max_states number of molecules
- [x] Test some large molecule returns a bunch of protomers
[x] Update docstrings/documentation, if applicable
[x] Lint codebase
[x] Update changelog
- [x] Document that input molecule cannot be counted on as returned

codecov[bot] commented 7 months ago

Codecov Report

Merging #1779 (84a5818) into main (34f666c) will decrease coverage by 0.21%. Report is 3 commits behind head on main. The diff coverage is 100.00%.

Additional details and impacted files

Yoshanuikabundi commented 6 months ago

The notebook I was messing around in: protomers.zip

The info in the review is probably more legible but it's here for posterity.

mattwthompson commented 6 months ago

Could you elaborate on why 2 is a waste of time? It's what I intuited the objective to be here. Is the issue that it might return a different number of states if the input molecule happens to be in what was returned by the underlying toolkit?

Yoshanuikabundi commented 6 months ago

I guess the issue is that if the input molecule isn't an important protomer, then when the user asks for the top n protomers and we give them those n plus the original molecule that might be a surprise (just like if they ask for the top n and we give them the top n minus the original). We need to decide what the desired behaviour is I guess.

j-wags commented 6 months ago

Talked a bit at my check-in with @mattwthompson about this. To move this forward, let's do the following:

Set the default max_states to 0
Not manually re-add the input to the list of outputs
Change the docstring to say that, if max_states is limited (set to something other than 0) the input molecule may not be returned