mosdef-hub / foyer

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

Bugfix for atomtyper #514

Closed daico007 closed 1 year ago

daico007 commented 1 year ago

PR Summary:

A type check was accidentally removed in a previous PR and causing some issue with the GMSO integration. This PR re-add said check, also update the variable name in the atomtyper methods to better reflect their purpose.

PR Checklist


lgtm-com[bot] commented 1 year ago

This pull request introduces 3 alerts when merging 4a01c60b638f50a18805217766190e615c705daf into 25a5fac50aa430899f7b21928dd8cad56085f61d - view on LGTM.com

new alerts:

codecov[bot] commented 1 year ago

Codecov Report

Merging #514 (0fa8921) into main (e81cb31) will decrease coverage by 0.06%. The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main     #514      +/-   ##
==========================================
- Coverage   70.17%   70.11%   -0.07%     
==========================================
  Files          16       16              
  Lines        1670     1673       +3     
==========================================
+ Hits         1172     1173       +1     
- Misses        498      500       +2     
bc118 commented 1 year ago

I think there is somethings wierd going on with the atom typer's (atomtyper.py) element

getting a new error for AA molecule:

Traceback (most recent call last):
  File "/Users/brad/Programs/anaconda3/envs/gmso_test_2/lib/python3.9/site-packages/mosdef_gomc/tests/run_test_charmm_writer/gmso_run_test_charmm_writer.py", line 10520, in <module>
    TestCharmmWriterData.test_save('', ethane_gomc)
  File "/Users/brad/Programs/anaconda3/envs/gmso_test_2/lib/python3.9/site-packages/mosdef_gomc/tests/run_test_charmm_writer/gmso_run_test_charmm_writer.py", line 253, in test_save
    Charmm(
  File "/Users/brad/Programs/anaconda3/envs/gmso_test_2/lib/python3.9/site-packages/mosdef_gomc/formats/gmso_charmm_writer.py", line 1575, in __init__
    ] = specific_ff_to_residue(
  File "/Users/brad/Programs/anaconda3/envs/gmso_test_2/lib/python3.9/site-packages/mosdef_gomc/utils/gmso_specific_ff_to_residue.py", line 502, in specific_ff_to_residue
    gmso_apply(new_gmso_topology,
  File "/Users/brad/Programs/anaconda3/envs/gmso_test_2/lib/python3.9/site-packages/gmso/parameterization/parameterize.py", line 104, in apply
    parameterizer.run_parameterization()
  File "/Users/brad/Programs/anaconda3/envs/gmso_test_2/lib/python3.9/site-packages/gmso/parameterization/topology_parameterizer.py", line 347, in run_parameterization
    typemap = self._get_atomtypes(
  File "/Users/brad/Programs/anaconda3/envs/gmso_test_2/lib/python3.9/site-packages/gmso/parameterization/topology_parameterizer.py", line 421, in _get_atomtypes
    foyer_topology_graph = get_topology_graph(
  File "/Users/brad/Programs/anaconda3/envs/gmso_test_2/lib/python3.9/site-packages/gmso/parameterization/foyer_utils.py", line 75, in get_topology_graph
    top_graph.add_atom(
  File "/Users/brad/Programs/anaconda3/envs/gmso_test_2/lib/python3.9/site-packages/foyer/topology_graph.py", line 89, in add_atom
    raise FoyerError(
foyer.exceptions.FoyerError: For atoms representing an element, please include both the atomic_number or symbol for the atom

input AA ethane:

def test_save(self, ethane_gomc):
    box_0 = mb.fill_box(
        compound=[ethane_gomc],
        n_compounds=[1],
        box=[4, 4, 4]
    )

    Charmm(
        box_0,
        "ethane",
        ff_filename="ethane",
        residues=[ethane_gomc.name],
        forcefield_selection="oplsaa",
    )

Error for UA two-propanol:

1515, in test_save_charmm_ua_gomc_ff
    charmm = Charmm(
  File "/Users/brad/Programs/anaconda3/envs/gmso_test_2/lib/python3.9/site-packages/mosdef_gomc/formats/gmso_charmm_writer.py", line 1575, in __init__
    ] = specific_ff_to_residue(
  File "/Users/brad/Programs/anaconda3/envs/gmso_test_2/lib/python3.9/site-packages/mosdef_gomc/utils/gmso_specific_ff_to_residue.py", line 502, in specific_ff_to_residue
    gmso_apply(new_gmso_topology,
  File "/Users/brad/Programs/anaconda3/envs/gmso_test_2/lib/python3.9/site-packages/gmso/parameterization/parameterize.py", line 104, in apply
    parameterizer.run_parameterization()
  File "/Users/brad/Programs/anaconda3/envs/gmso_test_2/lib/python3.9/site-packages/gmso/parameterization/topology_parameterizer.py", line 347, in run_parameterization
    typemap = self._get_atomtypes(
  File "/Users/brad/Programs/anaconda3/envs/gmso_test_2/lib/python3.9/site-packages/gmso/parameterization/topology_parameterizer.py", line 421, in _get_atomtypes
    foyer_topology_graph = get_topology_graph(
  File "/Users/brad/Programs/anaconda3/envs/gmso_test_2/lib/python3.9/site-packages/gmso/parameterization/foyer_utils.py", line 64, in get_topology_graph
    top_graph.add_atom(
  File "/Users/brad/Programs/anaconda3/envs/gmso_test_2/lib/python3.9/site-packages/foyer/topology_graph.py", line 94, in add_atom
    atom_data = AtomData(index, name, atomic_number, symbol, **kwargs)
TypeError: __init__() got multiple values for argument 'element'

Input for UA two-propanol:

def test_save_charmm_ua_gomc_ff_all_unique_atom_type_naming_style(self, two_propanol_ua):
    box_0 = mb.fill_box(
        compound=[two_propanol_ua],
        n_compounds=[1],
        box=[4, 4, 4]
    )

    charmm = Charmm(
        box_0,
        "charmm_UA_all_unique_atom_type_naming_style",
        ff_filename="charmm_UA_all_unique_atom_type_naming_style",
        residues=[two_propanol_ua.name],
        forcefield_selection="trappe-ua",
        bead_to_atom_name_dict={"_CH3": "C"},
    )
bc118 commented 1 year ago

Seems like something wierd going on with the element in both

This is the file I was using that worked for me:

atomtyper.py.zip

bc118 commented 1 year ago

OK, I feel like this is the exact same file that worked on my old version, and this file works if plugged my old foyer build.

What else changed (or did not change) in the last week in foyer that would break the foyer build.

Based on this, this PR is approved, but we need to figure out the issue posted above before pushing to the foyer conda package