-
Notifications
You must be signed in to change notification settings - Fork 2
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
implement isomerism bug fixes plus tests #20
Conversation
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.
Do we also need do the relative accounting for rs_isomers?
relative_attr: tuple(str, bool) | ||
a list of attributes that are sensetive | ||
to the ordering of nodes (i.e. refer to | ||
other nodes). The second element indicates | ||
the depth. If False the value of attr are | ||
node keys. If True we expect the value to | ||
be an itertable with node keys. |
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.
Instead of faffing around with this bool, you could see if you can do iter(thingy)
, and check that it's not a string.
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.
yeah I condiered that but then wasn't worth it ^^
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 downside of doing it the "clever" way is that there will be cases it gets it wrong (deeper nesting, for example). The downside of using the bool in the argument is that it complicates the api
@pckroon at the moment is is not required for the rs_isomer as the nodes only get annotated with '@' or '@@'. However, in the current code it is not possible to have the chiral center using the ring bond accounting. That's why it is not needed at the moment. The ring thing is a bit tricky because we just strip the smiles string of these characters. Doing the ring thing would actually require looking up the index belonging to a ring atom and matching it. |
Co-authored-by: Peter C Kroon <[email protected]>
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.
For the chiral ring things, is it possible to at least detect it and issue a warning?
…rrectly for implicit hydrogens
rings are fixed as long as they are within a fragment; chirality split between fragments remains a fragile thing |
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.
Looks good, some small nitpicks
No description provided.