-
Notifications
You must be signed in to change notification settings - Fork 33
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
Speedup identify_connections #741
Speedup identify_connections #741
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #741 +/- ##
=======================================
Coverage 91.99% 91.99%
=======================================
Files 67 67
Lines 6460 6460
=======================================
Hits 5943 5943
Misses 517 517
☔ View full report in Codecov by Sentry. |
Speeding this up will be great. Thanks for doing this! Have you bench marked it yet to get an idea of the performance improvement? |
I haven't but I can sweep up something |
Ran this code locally on a system. import gmso
import sys
sys.path.append("..")
from basic_testing import gmso_water_graph, box_water, box_polymer
import timeit
import numpy as np
setupStr = """
import gmso;
import sys;
sys.path.append("..");
from basic_testing import gmso_water_graph, box_polymer;
n_mols=100;
poly_box = box_polymer(n_mols);
gmso_top = gmso_water_graph(poly_box);
"""
outList = timeit.repeat("gmso_top.identify_connections()", setup=setupStr, number=3, repeat=3)
print(f"Identify {gmso_top.n_angles + gmso_top.n_dihedrals} Connection in"
+ f" {np.mean(outList):.2f} +- {1.96*np.std(outList)/np.sqrt(3):.3f} seconds"
) Output with this PR
Output with current GMSO main
Which is a a 25% speedup for this system |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. The performance improvement looks awesome, thanks @daico007!
I will merge this after all test passes, thanks for doing the profiling Cal! |
The current
Topology.identify_connections
method is quite slow for big system, this PR made several changes to thegmso/utils/connectivity.py
to improve the performance of said method (change the comparison of site to just their index, use IndexedSet instead of list, etc).