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

Sort indices in Frame.bond.group #769

Merged
merged 3 commits into from
Sep 22, 2023
Merged

Conversation

chrisjonesBSU
Copy link
Contributor

@chrisjonesBSU chrisjonesBSU commented Sep 21, 2023

This is a quick change that sorts the particle indices before appending to Frame.bond.group. This should have no effect on bond types or typeids. The bond.group is just a list of length 2 with the particle indices in the bond.

The reason why I think we should do this is because the locality and neighbor list modules in freud require that the bond arrays are sorted. Also, I think its cleaner and easier to read if you're ever having to read these arrays manually for some reason.

I'm not sure why this was never a problem for me when using the hoomd writers in mBuild, maybe its some how related to how Parmed indexes particles in bonds.

@CalCraven
Copy link
Contributor

This seems reasonable especially because getting things into a format for freud is good. Yeah it looks like things go to parmed systems. Could be a good test to make sure the mbuild snapshot writer handles this the same way as the gmso one.

@daico007
Copy link
Member

Would it be easier/faster to sneak this into #765? Or would these clash/out-of-scope?

@daico007
Copy link
Member

That #765 basically contains #762, which try to deal with all the sorting too

@codecov
Copy link

codecov bot commented Sep 21, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.02% ⚠️

Comparison is base (7537899) 92.82% compared to head (5192013) 92.81%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #769      +/-   ##
==========================================
- Coverage   92.82%   92.81%   -0.02%     
==========================================
  Files          66       66              
  Lines        6845     6845              
==========================================
- Hits         6354     6353       -1     
- Misses        491      492       +1     
Files Changed Coverage Δ
gmso/external/convert_hoomd.py 92.02% <ø> (ø)

... and 1 file with indirect coverage changes

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

@chrisjonesBSU
Copy link
Contributor Author

We could definitely just include this in #765

@daico007
Copy link
Member

I think the change is small enough, and that it has no impact on the other things (bond types and typeids), I think I gonna merge this and do a release of GMSO this morning. We could work on adding more tests later.

@daico007 daico007 merged commit 3dabe6a into mosdef-hub:main Sep 22, 2023
15 of 16 checks passed
@chrisjonesBSU chrisjonesBSU deleted the sort-bonds branch September 22, 2023 17:38
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