mosdef-hub / foyer

A package for atom-typing as well as applying and disseminating forcefields
https://foyer.mosdef.org
MIT License
121 stars 78 forks source link

XMLs distributed with foyer throw warnings #340

Closed rsdefever closed 3 years ago

rsdefever commented 4 years ago

Bug summary

When you load the forcefields distributed with foyer, e.g., ff = foyer.forcefields.load_OPLSAA(), three warnings are thrown:

  1. No force field version number found in force field XML file.
  2. No force field name found in force field XML file.
  3. You have empty smart definition(s)

It seems undesirable to have three warnings for the forcefields distributed with the package.

IMO, (1) and (2) are readily fixable -- we should add names and version numbers to the forcefields distributed with foyer. The latter is more challenging. One suggestion is to move the complete XML file to another (?) location and so that the XMLs distributed with foyer only contain atom types for which there are SMARTS definitions. What are others opinions on this?

Code to reproduce the behavior

import foyer
ff = foyer.forcefields.load_OPLSAA()

Software versions

chrisiacovella commented 4 years ago

I agree that 1 and 2 should be easy to fix.

As for number 3, I'm assuming that is the OPLS forcefield. It definitely is no fully annotated with SMARTS. One option for sure would be to move the "full" (but not fully SMARTed) forcefield to a separate repo, and only keep the parameters that have SMARTS.

We've discussed before whether or not it is good to have forcefields distributed as part of foyer. If you think of this like mbuild, we have a few example recipes, but in general, most recipes should be in their own repos as plugins. I think forcefields should be the same, we include some examples (e.g., a subset of parameters), that allows us to easily demonstrate functionality as well as to test the code as we modify it. So I'm in favor of hacking down the opls forcefield to just those types that are defined and have tests (assuming we also set up an opls repo for further development)

mikemhenry commented 4 years ago

+1 to having a fully smarted FF and move the full un-smarted FF xml into a separate repo.

mattwthompson commented 4 years ago

Tangential but consider stripping out built-in exceptions (errors and warnings) in favor of custom exceptions. This is more useful for the user (more descriptive and targetted, i.e. FoyerForcefieldVersionNotFound), developer (safer to catch a specific exception than i.e. TypeError which may be triggered by other causes, which is relevant for flow, tests, and custom warning profiles) and other APIs (i.e. if I build out a library that calls Foyer, such exceptions will clearly be in the scope of foyer, whereas a built-in exception could again be by other causes).