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

Change "OS" oxygen atoms in silica mol2 file to O #260

Merged
merged 1 commit into from Aug 6, 2019

Conversation

justinGilmer
Copy link
Contributor

PR Summary:

With the latest updates to mBuild in version 0.9.3, the atom names when
being converted to various file formats or parmed structures now undergo
a different process when their elements are being inferred.

Previously, an element name like OS, like in this silica.mol2 file
would evaulate to O, since OS is not an element.

This usually caused more issues though, because certain elements like
SI would be inferred as sulfur.

This now causes issues with some of our tests that rely on this silica
mol2 file, as OS is now being discovered as osmium.

The OS elements in the mol2 file have been renamed to O.

PR Checklist


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

With the latest updates to mBuild in version 0.9.3, the atom names when
being converted to various file formats or parmed structures now undergo
a different process when their elements are being inferred.

Previously, an element name like `OS`, like in this silica.mol2 file
would evaulate to O, since `OS` is not an element.

This usually caused more issues though, because certain elements like
`SI` would be inferred as sulfur.

This now causes issues with some of our tests that rely on this silica
mol2 file, as OS is now being discovered as osmium.

The OS elements in the mol2 file have been renamed to O.
@mattwthompson
Copy link
Member

If anybody were to compile a list of frustrations using biophysics software for materials simulations, the ambiguity of atom types/names/elements in mol2 files would make the cut

Copy link
Member

@mattwthompson mattwthompson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a big :%s/OS/O/g to me, skimming it I didn't find anything else

@justinGilmer
Copy link
Contributor Author

Thats pretty much what I did, just making sure not to override certain TRIPOS names to keep it as a valid mol2

@codecov
Copy link

codecov bot commented Aug 6, 2019

Codecov Report

Merging #260 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #260   +/-   ##
=======================================
  Coverage   85.76%   85.76%           
=======================================
  Files          10       10           
  Lines        1138     1138           
=======================================
  Hits          976      976           
  Misses        162      162

@ahy3nz
Copy link
Contributor

ahy3nz commented Aug 6, 2019

If anybody were to compile a list of frustrations using biophysics software for materials simulations, the ambiguity of atom types/names/elements in mol2 files would make the cut

"HA" and "CA" are atom names for some amino acids, then the mbuild to_parmed capitalization correction would convert to "Ha" and "Ca", then get tagged by parmed as Hahnium and Calcium, then ruin any sort of atomtyping. This probably just enforces the use of the underscore-virtual-element convention

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

3 participants