proteneer / timemachine

Differentiate all the things!
Other
138 stars 17 forks source link

Relax element coverage restriction #1361

Closed maxentile closed 1 month ago

maxentile commented 2 months ago

The current implementation of AM1CCCHandler does the following: "apply AM1, then apply BCCs that mostly clone the BCCs in OE impl." This will only recover the behavior of AM1BCC if the clone is an exact match. It is currently not an exact match: although the recharge project is very close, it does contain some gaps relative to the original publication (such as the element phosphorus) and some large mismatches from the OE current implementation of AM1BCC (e.g. on sulfones).

Continuing in the direction of https://github.com/proteneer/timemachine/pull/571, this PR proposes the logic: "apply AM1BCC, then apply arbitrary BCCs." This would (1) guarantee that we match the behavior of OE's current implementation of AM1BCC when the parameters are zeros, and (2) allow us to remove the assertion error that is now raised when mols contain phosphorus.

In detail:

By default, I lean toward this style of implementation: compose [trusted_starting_point(x), small_adjustment(x, theta)], rather than reimplementation_of_trusted_starting_point(x, theta). If we choose a different starting point in the future (such as an ML charge model), this would decouple our choice of starting point from our choice of how to learn any small adjustments to it.