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

Correctly set the mass when creating mb.Compound from a Snapshot; add mass_scale parameter #49

Merged
merged 19 commits into from
Jul 25, 2023

Conversation

chrisjonesBSU
Copy link
Member

I was getting some errors when trying to create a CG trajecotry of PPS. Ultimately, I think the errors resulted from a change in how mass is handled in mBuild. This PR sets the particle mass from the snapshot particle mass, and includes a mass_scale parameter for converting from reduced mass to real mass (similar to what was being done with length)

@codecov
Copy link

codecov bot commented Apr 3, 2023

Codecov Report

Merging #49 (7549f13) into main (9a5dc8d) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 7549f13 differs from pull request most recent head b2e8b72. Consider uploading reports for the commit b2e8b72 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #49      +/-   ##
==========================================
+ Coverage   96.62%   96.64%   +0.01%     
==========================================
  Files           4        4              
  Lines         385      387       +2     
==========================================
+ Hits          372      374       +2     
  Misses         13       13              
Files Changed Coverage Δ
grits/coarsegrain.py 98.20% <100.00%> (+<0.01%) ⬆️
grits/utils.py 96.77% <100.00%> (+0.03%) ⬆️

@RainierBarrett
Copy link
Contributor

It looks like we are getting the SMARTS strings wrong in this test, or borking the search somewhere. Running on the p3ht.gsd included in the repo gives this warning:

==============================
*** Open Babel Warning  in PerceiveBondOrders
  Failed to kekulize aromatic bonds in OBMol::PerceiveBondOrders (title is Compound)

/Users/cmelab/repos/grits/grits/coarsegrain.py:139: UserWarning: c1cscc1 not found in compound!
  warn(f"{smart_str} not found in compound!")
/Users/cmelab/repos/grits/grits/coarsegrain.py:139: UserWarning: CCC not found in compound!
  warn(f"{smart_str} not found in compound!")```

@RainierBarrett
Copy link
Contributor

Looks like it might be openbabel version updates causing the hiccup

@RainierBarrett
Copy link
Contributor

It looks like we are getting the SMARTS strings wrong in this test, or borking the search somewhere. Running on the p3ht.gsd included in the repo gives this warning:

==============================
*** Open Babel Warning  in PerceiveBondOrders
  Failed to kekulize aromatic bonds in OBMol::PerceiveBondOrders (title is Compound)

/Users/cmelab/repos/grits/grits/coarsegrain.py:139: UserWarning: c1cscc1 not found in compound!
  warn(f"{smart_str} not found in compound!")
/Users/cmelab/repos/grits/grits/coarsegrain.py:139: UserWarning: CCC not found in compound!
  warn(f"{smart_str} not found in compound!")```

This was happening because of us removing hydrogens erroneously before invoking grits, and without using the proper flag in the CG_System call. False alarm!

@chrisjonesBSU
Copy link
Member Author

Merging this for now since it is needed for #51 , we'll address the failing unit tests after this overhaul in finished up.

@chrisjonesBSU chrisjonesBSU merged commit 80c8380 into cmelab:main Jul 25, 2023
@chrisjonesBSU chrisjonesBSU deleted the pps branch July 25, 2023 17:03
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