[5] Writer #4
[5] Writer #4
Conversation
fgrunewald
force-pushed
the
write_cgsmiles
branch
from
5efc474 to
85b1c72
Compare
Co-authored-by: Peter C Kroon <[email protected]>
|
@pckroon I think the writer is rather complete now; I noticed that we probably should have a reader that returns the highest resolution molecule and fragment dicts. It is wrapped into the |
There was a problem hiding this comment.
Nice work! I do think I found a few small issues besides some minor formatting though. This also means those cases need to get tested.
I noticed that we probably should have a reader that returns the highest resolution molecule and fragment dicts. It is wrapped into the MoleculeResolver, but from a data science point of view, it could be handy to write and write the same stuff. I think it should be a separate PR however.
Yes and yes. Reading and writing the same thing is super useful, also from a testing point of view.
| if order_symb != '-': | ||
| bond_str = order_symb | ||
| bond_str += "["+str(bonding_descrpt[:-1])+"]" |
There was a problem hiding this comment.
Is it possible to elide aromatic bonds between aromatic atoms?
I prefer string formatting over concatenation personally:
| if order_symb != '-': | |
| bond_str = order_symb | |
| bond_str += "["+str(bonding_descrpt[:-1])+"]" | |
| if order_symb != '-': | |
| bond_str += order_symb | |
| bond_str += "[{}]".format(bonding_descrpt[:-1]) |
| def write_graph(molecule, smiles_format=False, default_element='*'): | ||
| """ | ||
| Creates a CGsmiles string describing `molecule`. | ||
| `molecule` should be a single connected component. |
There was a problem hiding this comment.
Meh. Not too much work to find all connected components and call a (this) function on those is it? And then just join them with a .
There was a problem hiding this comment.
😆 this one comes from the pysmiles parser I copied. It still has to be a connected component to be valid. That's simply a CGSmiles requirement. You can have edges with zero bond order though. But they have to be edges.
cgsmiles/write_cgsmiles.py
Outdated
| fragment_str = "" | ||
| for fragname, frag_graph in fragment_dict.items(): | ||
| fragment_str += f"#{fragname}=" | ||
| # format graph depending on resolution | ||
| fragment_str += write_graph(frag_graph, smiles_format=all_atom) + "," | ||
| fragment_str = "{" + fragment_str[:-1] + "}" |
There was a problem hiding this comment.
I would make a list of fragment_str's for the separate fragments, then ','.join them
There was a problem hiding this comment.
it doesn't matter does it?
| # we cannot be sure that the atomnames are the same because they | ||
| # will depend on the order | ||
| nx.set_node_attributes(frag_dict_out[fragname], None, "atomname") | ||
| nx.set_node_attributes(frag_dict[fragname], None, "atomname") |
There was a problem hiding this comment.
Can't we assume anything? For example that the first (two?) character should be the same?
There was a problem hiding this comment.
It is just the element plus the atom index. The element is already part of the testing. Therefore, I think it is not required.
Co-authored-by: Peter C Kroon <[email protected]>
This is a first draft of the writer for the CGSmiles.
To Do: