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

Remove deprecated formats, update reader and writer backends #1191

Merged
merged 46 commits into from
Oct 26, 2024

Conversation

chrisjonesBSU
Copy link
Contributor

PR Summary:

This PR gets things started to remove the deprecated items talked about in #1187

Also, this will update the reader/writer backends to use GMSO where ever possible.

PR Checklist


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

@chrisjonesBSU chrisjonesBSU marked this pull request as ready for review July 2, 2024 20:29
@chrisjonesBSU
Copy link
Contributor Author

The failing tests are in test_xyz.py. The behavior we expect in the test is not what we get when using the gmso backend workflow. If we load a single molecule with infer_hierarchy = True do we expect the particles of the created compound to be children (the way the test is currently written), or do we expect a flat molecule (what we get from gmso)?

Also, if I change infer_heirarchy=False when using conversion.py the behavior is different from using GMSO directly and passing infer_hierarchy=False to from_mbuild.

Here is a gist showing what I'm trying to say here.

@CalCraven
Copy link
Contributor

To organize my thoughts, the four use cases are:

  1. Use mBuild to load xyz, infer_hierarchy=True
  2. Use mBuild to load xyz, infer_hierarchy=False
  3. Use GMSO to load xyz, infer_hierarchy=True
  4. Use GMSO to load xyz, infer_hierarchy=False

As @chrisjonesBSU has in his gist, 1-3 gives a layered compound with 1 child, which then itself has 8 children particles. Method 4 gives a flat compound with 8 children.

We should agree on preferred behavior. I actually think the current output from Method 4 is what I would expect for all compounds made from xyz files. Do you agree or disagree @chrisjonesBSU?

My reasoning is since there is no hierarchy present in an xyz file, I would expect a consistent flat compound. Infer_hierarchy therefore should not make a difference either way, as what happens in the current mBuild methods.

In order to obtain this behavior, I think we could either tweak the to_mbuild function so that any topologies without molecule or residue information give a single compound with multiple particles. To me, that seems like the preferred way to convert to a compound without extra hierarchy info. However, we need to test that, as it could screw any behavior from other readers if we modify the to_mbuild method, which every other conversion pathway also takes.

An optional workaround is to just set the molecule/residue information when reading in xyz files such that the to_mbuild can be kept the same, but gives us the preferred outcome. Since that info is not really part of the xyz file, I think manually setting it in the xyz reader in a generalized manner is a way we get consistent results. It would need to be set such that we get the flat compound when infer_hiearchy=True, so gotta dig into that bit of the to_mbuild function still.

@chrisjonesBSU
Copy link
Contributor Author

chrisjonesBSU commented Aug 26, 2024

Still need to remove lampsdata.py and lamps unit tests.

@chrisjonesBSU
Copy link
Contributor Author

chrisjonesBSU commented Aug 29, 2024

There seems to be something funny going on with pytest and how it is caching things. test_resnames_parmed_cg is failing; however, if you just run that test only without running everything else in test_compound.py it passes.

pytest test_compound.py -k test_resnames_parmed_cg

Another way to test this is to disable cacheing

pytest test_compound.py -p no:cacheprovider

Then all 182 tests in test_compound.py pass.

Copy link

codecov bot commented Sep 13, 2024

Codecov Report

Attention: Patch coverage is 70.58824% with 10 lines in your changes missing coverage. Please review.

Project coverage is 85.93%. Comparing base (38077cd) to head (bbffe82).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
mbuild/conversion.py 67.85% 9 Missing ⚠️
mbuild/compound.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1191      +/-   ##
==========================================
- Coverage   87.35%   85.93%   -1.43%     
==========================================
  Files          62       53       -9     
  Lines        6588     4898    -1690     
==========================================
- Hits         5755     4209    -1546     
+ Misses        833      689     -144     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mbuild/formats/hoomd_writer.py Fixed Show fixed Hide fixed
mbuild/formats/hoomd_writer.py Fixed Show fixed Hide fixed
mbuild/formats/hoomd_writer.py Fixed Show fixed Hide fixed
mbuild/conversion.py Fixed Show fixed Hide fixed
@chrisjonesBSU
Copy link
Contributor Author

@CalCraven we previously discussed about still having support for saving .lammps and .lammpsdata files directly from mBuild, but the wrtiers we have in GMSO for this only work with typed topologies. So, for now I removed these file types from tests.

@chrisjonesBSU
Copy link
Contributor Author

The windows 3.12 test should pass once conda-forge/gmso-feedstock#27 is merged and available.

The windows 3.12 run is sort of unique, it has to use the latest version of GMSO because of a PR fixed related to importing hoomd, but the latest GMSO doesn't support python 3.12. Some of the older versions must not have pinned a maximum python version, which is why the other 3.12 tests are passing.

mbuild/tests/test_compound.py Fixed Show fixed Hide fixed
mbuild/tests/test_rigid.py Fixed Show fixed Hide fixed
mbuild/utils/io.py Dismissed Show dismissed Hide dismissed
mbuild/tests/test_compound.py Dismissed Show dismissed Hide dismissed
@chrisjonesBSU
Copy link
Contributor Author

@CalCraven This should be ready to go.

Copy link
Contributor

@CalCraven CalCraven left a comment

Choose a reason for hiding this comment

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

Few more minor comments actually, mostly about some doc strings.

mbuild/compound.py Outdated Show resolved Hide resolved
**kwargs
Depending on the file extension these will be passed to either
`write_gsd`, `write_hoomdxml`, `write_lammpsdata`, `write_mcf`, or
`write_gsd`, `write_lammpsdata`, `write_mcf`, or
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to pass the following conversion mapping in the documentation. Then we just link to

def save(
for more details?

`{GMSO: {".gro", ".gsd". ".data", ".xyz", ".mcf", ".top"},
ParmEd: {".mol2", ".pdb", ".prmtop", ".cif", ".crd"},
PyBel:{".sdf"}}`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. This should be done, along with the other suggestions.

mbuild/compound.py Outdated Show resolved Hide resolved
mbuild/compound.py Show resolved Hide resolved
mbuild/conversion.py Outdated Show resolved Hide resolved
@CalCraven CalCraven merged commit aabbab6 into mosdef-hub:main Oct 26, 2024
19 of 21 checks passed
@chrisjonesBSU chrisjonesBSU deleted the remove-dep branch October 27, 2024 21:54
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.

2 participants