improve 2d vis of mappings #278
Conversation
previously would independently calculate 2D vis for each molecule, then RMS align these now constrains second molecule's 2d representation to match that of the first molecule overall improves the 2d vis generated, esp. when dealing with rotamers
|
Hello @richardjgowers! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-01-29 13:47:29 UTC |
| The structure contains the indices of the molecules in mols as key | ||
| Tuple[molA, molB] and maps the atom indices via the value Dict[ | ||
| molA_atom_idx, molB_atom_idx] | ||
| atom_mapping: dict[int, int], optional |
There was a problem hiding this comment.
I'm changing the signature of this, since it's an private/internal function. Previously it would accept arbitrarily many molecules and mapping, but this was untested/unused. I've narrowed this down to 1 molecule or 2 molecules with a mapping.
There was a problem hiding this comment.
would we need to turn this back, when going multistate?
There was a problem hiding this comment.
or would we implement a new function?
There was a problem hiding this comment.
just a thought, to think about it.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #278 +/- ##
=======================================
Coverage 99.22% 99.22%
=======================================
Files 36 36
Lines 1938 1942 +4
=======================================
+ Hits 1923 1927 +4
Misses 15 15 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Would it be possible to see an example with a larger R group please? So we know that the hydrogen "squishing" isn't going to lead to more worse visual problems long term?
edit: for example, what if the hydrogen at position 10/12 on the benzene right is actually COOH?
| atomMap=[(k, v) for v, k in atomMap.items()] | ||
| ) | ||
| # create layout of first molecule | ||
| # TODO: |
There was a problem hiding this comment.
Please raise an issue for this current problem and this todo.
I suspect someone's going to stumble upon this with an old version of gufe and we'll forget this ever happened.
related to off tk #1563
related to off tk #1563
This reverts commit 812b7be.
previously would independently calculate 2D vis for each molecule, then RMS align these
now constrains second molecule's 2d representation to match that of the first molecule
overall improves the 2d vis generated, esp. when dealing with rotamers
before: (here atom 9L maps to atom 9R. rdkit however assigns a more traditional sulfonamide layout)
after: (atom 9L & 9R are constrained. This results in the H's having less layout room, but makes it clear why they didn't map)