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

Use list to store compound.children #1121

Merged
merged 1 commit into from
May 31, 2023

Conversation

daico007
Copy link
Member

PR Summary:

After the discussion with @chrisiacovella, we have decided it would be faster to store Compound.children with list instead of the in house OrderedSet. The replacement will speed up both the Compound.add step by removing the comparing stage when adding new children to a set, and the indexing (getting element from a list/set by index). We realized the latter was the cause for #1114, not directly the Compound.add_bond itself, but how the particle is retrieved from Compound.children to perform the addition.

PR Checklist


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

@daico007
Copy link
Member Author

@chrisiacovella, I kept the containment intact, since it's still being used by the polymer.build, and if it's false, the children is not being added anyway. The in-house OrderedSet is kept since we are still using it in the lammpsdata.py, and it's technically a modified OrderedSet with some extra property.

@codecov
Copy link

codecov bot commented May 30, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.02 ⚠️

Comparison is base (87edc4a) 87.15% compared to head (acabea0) 87.13%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1121      +/-   ##
==========================================
- Coverage   87.15%   87.13%   -0.02%     
==========================================
  Files          62       62              
  Lines        6422     6420       -2     
==========================================
- Hits         5597     5594       -3     
- Misses        825      826       +1     
Impacted Files Coverage Δ
mbuild/coarse_graining.py 92.30% <100.00%> (-0.07%) ⬇️
mbuild/compound.py 97.11% <100.00%> (-0.01%) ⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@chrisiacovella chrisiacovella left a comment

Choose a reason for hiding this comment

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

This looks good!

@daico007 daico007 merged commit 12efb1b into mosdef-hub:main May 31, 2023
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