-
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
Turn off identify_connected_components default #760
Turn off identify_connected_components default #760
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #760 +/- ##
==========================================
+ Coverage 92.06% 92.07% +0.01%
==========================================
Files 67 67
Lines 6892 6890 -2
==========================================
- Hits 6345 6344 -1
+ Misses 547 546 -1
☔ View full report in Codecov by Sentry. |
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.
Per this discussion, a few other changes may as well be made here:
identify_connected_components: bool, optional, default=False
A flag which will speed up atomtyping for systems with many repeated molecules in
the system. Specifically useful if `top.unique_site_labels()` isn't properly identifying
repeated. However, if the molecules are identified, then it is recommended to use
use_molecule_info.
molecules.
Also, I would set use_molecule_info to True as default, since that shouldn't add any overhead, since it's just checking the molecule tags. The use_molecule_info text should also be changed. Maybe something like:
A flag to determine whether or not to look at site.molcule_name to look for repeated molecules, and copy the parameters out to the remaining molecules.
I was also thinking we could rename these parameters. There are three potential speedup methods:
identify_connected_components=True
use_molecule_info=True
fast_copy=True
What if we named them something something more cohesive so it was clear if things are running slow for you, you should try to use of of these flags?
Something:
speedup_idmolecules
speedup_molecule_tag
speedup_copyexpressions
While we're here, what do we think about reformatting assert_x_parameters as well. What if we condensed it to assert_parameter_connections, and you pass a dict instead of 4 different bools? Might reduce how lengthy this function docstring is.
I like the idea of using |
Good idea! I think we can go with |
Regarding the |
I included the proposed changes in the latest commit, lmk what y'all think. |
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.
I left a couple comments for some small changes.
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.
Once those few changes are in, this looks good to me.
…_default_option_for_apply
I should have addressed all the comments by @chrisjonesBSU and @CalCraven in the last commit. |
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! I made one commit to change True to False in the doc string for speedup_by_molgraph
Few more minor doc string changes and this is good to merge. |
Co-authored-by: CalCraven <[email protected]>
Co-authored-by: CalCraven <[email protected]>
Per discussion stemmed from #757, this PR turned off the default
identify_connected_components
option in theparameterize.apply
.