Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix for atomtyper #514

Merged
merged 3 commits into from
Sep 7, 2022
Merged

Bugfix for atomtyper #514

merged 3 commits into from
Sep 7, 2022

Conversation

daico007
Copy link
Member

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


  • Includes appropriate unit test(s)
  • Appropriate docstring(s) are added/updated
  • Code is (approximately) PEP8 compliant
  • Issue(s) raised/addressed?

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Aug 31, 2022

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

new alerts:

  • 3 for Wrong name for an argument in a call

@codecov
Copy link

codecov bot commented Sep 1, 2022

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
Copy link

bc118 commented Sep 7, 2022

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
Copy link

bc118 commented Sep 7, 2022

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
Copy link

bc118 commented Sep 7, 2022

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

@daico007 daico007 merged commit 39c84a2 into mosdef-hub:main Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants