Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Atomtype parameterization methods for parameterizing a topology in GMSO. #644
Atomtype parameterization methods for parameterizing a topology in GMSO. #644
Changes from 2 commits
8e69d6e
9074909
80c1850
6376da7
7d0a790
99c0793
790aacc
2d7a770
e6da3f7
40f6639
e1f3d4e
1503578
b13bf94
7be5f57
7366452
cc46491
c58aa41
0f374b2
f5633e7
d7a90b0
72137ee
263ef78
c7ea346
d267d2f
9aab528
51b1081
06df2ba
d7b08d6
031e703
4a523e6
403a36c
593c15a
341ecc5
fe352fd
b3e045f
4834b5f
0a4c780
5633c52
3711172
956977c
f0fd1e5
1e866d7
da3a23e
b018a6c
4c62c5c
318c5ad
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 think we need to add a node_match function (based on the element of the node) IIRC.
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.
What exactly would a node_match function do? I'm not entirely familiar, is it something passed to the GraphMatcher to do the node matching?
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.
just tested this offline, I think the current use of GraphMatcher only concern about the shape of the graph but not about the nodes involved. So this could cause issue with molecules with same shape but different element would still be recognized as isomorphic (so lumped into the same group).
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.
Note below I am using the
get_modified_topology_graph
from the example notebook.Then
matcher.is_isomorphic()
will still returnTrue
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.
on a different note, I think we can use this
GraphMatcher
for the issue with the assumed site order in a molecule (or subtopology) (also this mosdef-hub/foyer#504). Will have to sacrifice some performance for checking but the number of checks performed will still be fewer that going the completely splitting the topology, so should still be a bit faster.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.
Okay this makes more sense to me now, thanks @daico007. I was a little hazy on the exact implementation of GraphMatcher. I think having a few different options for matching makes sense, which trade of increasing performance for increasing matching precision. This reminds me a bit of the filters used in PR #649, which can be used to specify how you want to filter unique parameter types. I think it makes sense to at least try a basic one that checks at least for elements in the correct positions, and allow that to be a built in option.
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.
Any thoughts on adding a flag for this operation?
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.
So only identify connections if those connections were missing? Makes sense to me, do you want it default flagged on or off?
I do start to get worried about the number of flags that are starting to get passed around.
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 think we will need a flag for this operation if we want to keep it here. But, I am thinking, should we ask the user to perform the
identify_connection
before arriving at this method? Since theidentify_connection
itself may need some adjustment (like do we want to identify improper dihedrals or not).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.
Can you elaborate on the question posed in this comment?
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.
This question was a note to myself to handle the case that only one forcefield is passed, without specifying the molecules it should be attached to. Currently, that will just spit out an error since it expects
forcefields
to be of type dict. So we should either tell users in this case to use identify_connected_components=False, or just automatically use the single forcefield that was passed.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 don't think this will work for a dictionary of forcefields with more than one key right?
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.
Not sure where we want to address this but we need a better identifier than
atom.label
.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 agree. I will think about this, and see if there is something better we can be using.
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.
Should not the atom be the atom number in the topology?
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.
The index is the atom_number in the topology. The subtopology_label should be the string that we use to apply a given forcefield to the sites with this label.
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.
According to PR #638, this info will be stored in site.molecule. I think whatever convention we decide to use there can be the reference label we use to identify the molecule name, and therefore the identifier we use for the forcefield
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.
Is a None-Type check prudent here? I think foyer will raise an error if it cannot find an atomtype right?
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.
Would we want to use the assert_bond_params flag to see if we should or should not look for all instances of that connection to be typed? This will definitely throw an error if it's missing as of now. Line 97 essentially handles this check as of now.
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.
Added an error message in e1f3d4e.