SKETCH-829 #54
SKETCH-829 #54
Conversation
only assign wedges if the center canBeChiral()
|
@d-b-w let's discuss appropriate tests for this and CRDGEN-250 |
sketcherMinimizerAtom.cpp
Outdated
| @@ -389,7 +389,7 @@ void sketcherMinimizerAtom::writeStereoChemistry() // sets stereochemistry for | |||
| return; | |||
| } | |||
| size_t n = neighbors.size(); | |||
| if (n != 3 && n != 4) { | |||
| if (!canBeChiral() || (n != 3 && n != 4)) { | |||
There was a problem hiding this comment.
Wait - does this mean that the carbon atoms in the JIRA case aren't going to be marked as chiral at all? That seems wrong, too.
There was a problem hiding this comment.
well they are, and it makes sense cause they have no Hs. We have another case to track that problem so in the end they won't, but as far as this case is concerned I think this is the appropriate behaviour for coordgen
There was a problem hiding this comment.
Sorry, I'm not sure I understand. The C atoms may not have any explicit H atoms, but implicit ones should also be considered, shouldn't they?
There was a problem hiding this comment.
So, I guess when I see this I have a couple of questions:
- What is
(n != 3 && n != 4)even doing? Isn't that logic already performed inCanBeChiral()? - Should there be some earlier check or assertion that we're not setting stereochemistry on something that can't even be chiral in the first place?
- What about trigonal nitrogens? Do you allow them to be have parity?
There was a problem hiding this comment.
@ricrogz the problem is that we were marking the atoms to have 0 implicit Hs. So when I have no Hs I mean no implicit and no explicit Hs.
There was a problem hiding this comment.
@d-b-w 1) you are right
2) this looks like the right place to check to me. It's at the beginning of where we write stereochemistry to the atom
3) yes
There was a problem hiding this comment.
It sounds like you're curious about how to add tests here. I am too!
Would it be possible to add a mae file that represents this? I guess you don't parse chirality/parity information out of mae files, so maybe it would be hard to generate a test case for this.
Would it be possible to add a test in the core suite that exercises the type of problem in the case? It's really a problem in the communication between coordgen and the core suite, right?
sketcherMinimizerAtom.cpp
Outdated
| @@ -389,7 +389,7 @@ void sketcherMinimizerAtom::writeStereoChemistry() // sets stereochemistry for | |||
| return; | |||
| } | |||
| size_t n = neighbors.size(); | |||
| if (n != 3 && n != 4) { | |||
| if (!canBeChiral() || (n != 3 && n != 4)) { | |||
There was a problem hiding this comment.
So, I guess when I see this I have a couple of questions:
- What is
(n != 3 && n != 4)even doing? Isn't that logic already performed inCanBeChiral()? - Should there be some earlier check or assertion that we're not setting stereochemistry on something that can't even be chiral in the first place?
- What about trigonal nitrogens? Do you allow them to be have parity?
Removes duplicate check
|
looks like we've misplaced valgrind. @ricrogz - do you remember what we did last time that happened? |
sketcherMinimizerAtom.cpp
Outdated
| @@ -389,7 +389,7 @@ void sketcherMinimizerAtom::writeStereoChemistry() // sets stereochemistry for | |||
| return; | |||
| } | |||
| size_t n = neighbors.size(); | |||
There was a problem hiding this comment.
It's not valgrind, @d-b-w: the build failed (and everyting after it) because n is now unused.
address build failure
canBeChiral(), among other things, makes sure that we set the number of implicit Hs correctly on the stereocenter. If it fails we should not add wedges and dashes