mosdef-hub / foyer

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

Metadata parsing issues #434

Closed mattwthompson closed 3 years ago

mattwthompson commented 3 years ago

Bug summary

Metadata is not consistently being parsed when force field files are loaded. See this thread and some comments before and after it. This blocks #433 and any future metadata additions.

Code to reproduce the behavior

In:

import foyer

oplsaa = foyer.forcefields.load_OPLSAA()
try:
    assert oplsaa.name == "OPLS-AA"
except AssertionError:
    print("load_OPLSAA() did not grab metadata.")

oplsaa = foyer.Forcefield(name="oplsaa")
try:
    assert oplsaa.name == "OPLS-AA"
except AssertionError:
    print("Forcefield(name=\"oplsaa\") did not grab metadata.")
oplsaa = foyer.Forcefield(forcefield_files="foyer/forcefields/xml/oplsaa.xml")
try:
    assert oplsaa.name == "OPLS-AA"
except AssertionError:
    print("Forcefield(forcefield_files=...) did not grab metadata.")

Out:

/Users/mwt/software/foyer/foyer/validator.py:165: ValidationWarning: You have empty smart definition(s)
  warn("You have empty smart definition(s)", ValidationWarning)
Forcefield(name="oplsaa") did not grab metadata.

Software versions

ahy3nz commented 3 years ago

https://github.com/mosdef-hub/foyer/blob/3b9108247025f11b5614f4c48c0c5418b1e9c7ae/foyer/forcefield.py#L531-L538

This logic is dependent on the forcefield_files arg, but I think it should use all_files_to_load variable, since it always gets populated in this code. In the name=oplsaa case, the forcefield_files var is never populated, but the all_files_to_load var DOES get populated. In the other two cases, the forcefield_files var gets populated, but so does the all_files_to_load var.

If you replace those lines with all_files_to_load (:530,538s/forcefield_files/all_files_to_load/g), the metadata will be parsed in all 3 cases, but all will return List[str] and not str. I think changing those 7 lines a possible, quick-fix solution to this metadata parsing issue


        if isinstance(all_files_to_load, str):
            self._version = self._parse_version_number(all_files_to_load)
            self._name = self._parse_name(all_files_to_load)
        elif isinstance(all_files_to_load, list):
            self._version = [
                self._parse_version_number(f) for f in all_files_to_load
            ]    
            self._name = [self._parse_name(f) for f in all_files_to_load]