Molecule/apply link #241
Molecule/apply link #241
Conversation
There was a problem hiding this comment.
I like the functionality, but I'm not sure I like the implementation. Making it a method of Molecule makes Molecule and Link much tighter coupled, which may cause maintenance issues down the line. I think it would make more sense as a function in do_links.py (but this can be discussed). Beyond that I think it can be simplified by splitting it into two functions: one which will take a molecule, a link, and a match (dict) and will just apply the link to those atoms. This function should be copy-pastable from do_links.py. A second function could then generate a match for a given pair or residues and molecule. This would also solve the _build_link_interaction location issue.
I'm hedging on the ignore_keys keyword. I'm not quite sure it actually adds functionality. An argument against is that you can no longer look for atoms based on the attribute ignore_keys (which is not the case for attributes_match, since that doesn't do the ** unpacking). Either way, your current hackaround is very much a hack. Especially since it's easy to just strip the offending attributes from the dict.
From a usability point of view, I'm not sure ignoring 'order' is desirable, what's your argumentation?
Lastly some nitpicks: Run Pylint, some trailing whitespace appeared; and don't commit the .coverage files. Add .coverage* to .gitignore if you want.
| @@ -757,7 +763,7 @@ def same_interactions(self, other): | |||
| # instance, the assumed default for the `PTM_atom` attribute is False. | |||
| def same_nodes(self, other, ignore_attr=()): | |||
| """ | |||
| Returns `True` if the nodes are the same and in the same order. | |||
| Returnsignore = if the nodes are the same and in the same order. | |||
| Applies a link between specific residues, if and only if | ||
| the link atoms incl. all attributes match at most one atom | ||
| in a respective link. |
There was a problem hiding this comment.
Link atoms always match link atoms, so something in this docstring doesn't make sense.
Also, at most one atom, or at least one atom?
| # parked this here because importing it seems to fail apparently | ||
| # because some relative references are used in do_links.py | ||
| def _build_link_interaction_from(molecule, interaction, match): | ||
| atoms = tuple(match[idx] for idx in interaction.atoms) | ||
| parameters = [ | ||
| param(molecule, match) if callable(param) else param | ||
| for param in interaction.parameters | ||
| ] | ||
| new_interaction = interaction._replace( | ||
| atoms=atoms, | ||
| parameters=parameters | ||
| ) | ||
| return new_interaction |
There was a problem hiding this comment.
This should probably be a method of Link, or just be a helper function in the right place
| # we have to go on resid or at least one criterion otherwise | ||
| # the matching will be super slow, if we need to iterate | ||
| # over all combintions of a possible links. | ||
| nx.set_node_attributes(link, dict(zip(link.nodes, resids)), 'resid') |
There was a problem hiding this comment.
Make a copy of link before doing this.
Also, since link.nodes is a dict, this depends on dict ordering. Which is bad.
Also2, if link nodes had resid attributes, those get clobbered.
| link_to_mol = {} | ||
| for node in link.nodes: | ||
| attrs = link.nodes[node] | ||
| attrs.update({'ignore':['order']}) |
There was a problem hiding this comment.
Instead just remove the order attibute from the attrs.
| else: | ||
| msg = "Found no matchs for atom {} in resiue {}. Cannot apply link." | ||
| raise KeyError(msg.format(attrs["atomname"], attrs["resid"])) |
There was a problem hiding this comment.
No chance of more than 1 match?
| new_edges = [(link_to_mol[atoms[i]], link_to_mol[atoms[i+1]]) for i in | ||
| range(0, len(atoms)-1)] |
There was a problem hiding this comment.
... for at1, at2 in zip(atoms[:-1], atoms[1:]) instead of indexing.
| """ | ||
| Test that force field files are properly read. | ||
| """ |
| @@ -0,0 +1,131 @@ | |||
| # Copyright 2018 University of Groningen | |||
|
Let's stash this for now as we are not really sure yet, which use cases might be underlying and it is not so clear what the method should do and where it should be present in the code. |
Hi,
This is my take on a
apply_linksfunction as mentioned in issue #239. In principle this is a very simple procedure. You feed it a molecule and a link as well as the two resids to which the link applies. Subsequently the atoms are matched based on those criteria as done in the ApplyLinks processor. In the end I decided to require the resid input because in the context of a vermouth molecule a resid is unique and it becomes impractical to just take two resnames and build all combinations of all valid residues and see, if all interactions match. I think in the end when you want to force a link you will already know the resid within the molecule. In connection with a higher resolution graph as will be used by polyply this allows an easy way to just put links if you know certain residues are connected.Known Issues:
it will put a link that was input as
BB BB some paramswhere both BB atoms have the same resid. This means it will for example for a diatomic molecule build a bond0 0 some params. But this should be filtered at the input level in my opinion.I parked a function from the DoLinks in the class method, because I couldn't make vermouth call that function. @pckroon maybe you have an idea how to do this.
find_atomsshould in my opinion also take theignore_keyskwarg of the match method. At the moment I added a workaround so that you can feed it as through**attrs. In this way nothing else breaks. If I am correct the method is only called in tests. So fixing it might be easy. But let's see if anyone has an opinion on this.Cheers,
Fabian