Skip to content

Commit 834d1eb

Browse files
johnmayjohnmay
authored
Merge pull request cdk#1226 from uli-f/doublebondstereochemistry-create-alignment-fix
swap carrier order in DoubleBondStereochemistry::create to satisfy bond alignment constraint
2 parents f14d486 + a5e7588 commit 834d1eb

4 files changed

Lines changed: 111 additions & 24 deletions

File tree

‎base/core/src/main/java/org/openscience/cdk/stereo/AbstractStereo.java‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,13 +49,26 @@ protected static int numCarriers(int cfg) {
4949
return ((cfg >>> 12) & 0xf);
5050
}
5151

52+
/**
53+
* Hook for subclasses to process carriers before they are stored, e.g., to enforce a constraint.
54+
* The default implementation returns the carriers as-is.
55+
*
56+
* @param focus the focus
57+
* @param carriers the carriers
58+
* @return the processed carriers
59+
*/
60+
protected C[] processCarriers(F focus, C[] carriers) {
61+
return carriers;
62+
}
63+
5264
AbstractStereo(F focus, C[] carriers, int value) {
5365
if (focus == null)
5466
throw new NullPointerException("Focus of stereochemistry can not be null!");
5567
if (carriers == null)
5668
throw new NullPointerException("Carriers of the configuration can not be null!");
5769
if (carriers.length != numCarriers(value))
5870
throw new IllegalArgumentException("Unexpected number of stereo carriers! expected " + ((value >>> 12) & 0xf) + " was " + carriers.length);
71+
carriers = processCarriers(focus, carriers);
5972
for (C carrier : carriers) {
6073
if (carrier == null)
6174
throw new NullPointerException("A carrier was undefined!");

‎base/core/src/main/java/org/openscience/cdk/stereo/DoubleBondStereochemistry.java‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,27 @@ public DoubleBondStereochemistry(IBond stereoBond, IBond[] ligandBonds, int conf
5454
super(stereoBond, ligandBonds, CT | (CFG_MASK & config));
5555
}
5656

57+
/**
58+
* Processes the given carrier bonds to ensure the first carrier is connected to the
59+
* beginning atom of the focus bond. Swaps the carriers if this condition is not met.
60+
*
61+
* @param focus the focus bond whose beginning atom should be connected to the first carrier bond
62+
* @param carriers an array of two carrier bonds associated with the stereochemistry,
63+
* where the first bond should connect to the beginning atom of the focus bond
64+
* @return an array of carrier bonds where the above constraint is satisfied
65+
*/
66+
@Override
67+
protected IBond[] processCarriers(IBond focus, IBond[] carriers) {
68+
// Ensure that the constraint carrier[0] connected to focus.getBegin()
69+
// is satisfied
70+
if (carriers[0] != null && focus != null && focus.getBegin() != null &&
71+
carriers[0].getOther(focus.getBegin()) == null) {
72+
// Swap carriers if the condition above is not satisfied.
73+
return new IBond[]{carriers[1], carriers[0]};
74+
}
75+
return carriers;
76+
}
77+
5778
public void setBuilder(IChemObjectBuilder builder) {
5879
super.setBuilder(builder);
5980
}

‎base/test-core/src/test/java/org/openscience/cdk/stereo/DoubleBondStereochemistryTest.java‎

Lines changed: 74 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -26,26 +26,21 @@
2626
import org.junit.jupiter.api.BeforeAll;
2727
import org.junit.jupiter.api.Test;
2828
import org.openscience.cdk.Atom;
29-
import org.openscience.cdk.test.CDKTestCase;
3029
import org.openscience.cdk.DefaultChemObjectBuilder;
31-
import org.openscience.cdk.interfaces.IAtom;
32-
import org.openscience.cdk.interfaces.IAtomContainer;
33-
import org.openscience.cdk.interfaces.IBond;
30+
import org.openscience.cdk.exception.CDKException;
31+
import org.openscience.cdk.interfaces.*;
3432
import org.openscience.cdk.interfaces.IBond.Order;
35-
import org.openscience.cdk.interfaces.IChemObjectBuilder;
36-
import org.openscience.cdk.interfaces.IDoubleBondStereochemistry;
3733
import org.openscience.cdk.interfaces.IDoubleBondStereochemistry.Conformation;
34+
import org.openscience.cdk.silent.SilentChemObjectBuilder;
35+
import org.openscience.cdk.smiles.SmilesParser;
36+
import org.openscience.cdk.test.CDKTestCase;
3837

3938
import java.util.Collections;
4039
import java.util.HashMap;
4140
import java.util.Map;
4241

43-
import static org.hamcrest.CoreMatchers.is;
44-
import static org.hamcrest.CoreMatchers.not;
45-
import static org.hamcrest.CoreMatchers.sameInstance;
42+
import static org.hamcrest.CoreMatchers.*;
4643

47-
/**
48-
*/
4944
class DoubleBondStereochemistryTest extends CDKTestCase {
5045

5146
private static IAtomContainer molecule;
@@ -55,7 +50,7 @@ class DoubleBondStereochemistryTest extends CDKTestCase {
5550
* This method creates <i>E</i>-but-2-ene.
5651
*/
5752
@BeforeAll
58-
static void setup() throws Exception {
53+
static void setup() {
5954
molecule = DefaultChemObjectBuilder.getInstance().newAtomContainer();
6055
molecule.addAtom(new Atom("C"));
6156
molecule.addAtom(new Atom("C"));
@@ -129,7 +124,7 @@ void testGetBonds() {
129124
}
130125

131126
@Test
132-
void contains() throws Exception {
127+
void contains() {
133128
IChemObjectBuilder builder = DefaultChemObjectBuilder.getInstance();
134129

135130
IAtom c1 = builder.newInstance(IAtom.class, "C");
@@ -182,7 +177,7 @@ void testMap_Map_Map() throws CloneNotSupportedException {
182177
mapping.put(c2o4, c2o4clone);
183178

184179
// map the existing element a new element
185-
IDoubleBondStereochemistry mapped = original.map(Collections.EMPTY_MAP, mapping);
180+
IDoubleBondStereochemistry mapped = original.map(Collections.<IAtom,IAtom>emptyMap(), mapping);
186181

187182
org.hamcrest.MatcherAssert.assertThat("mapped chiral atom was the same as the original", mapped.getStereoBond(),
188183
is(not(sameInstance(original.getStereoBond()))));
@@ -191,19 +186,19 @@ void testMap_Map_Map() throws CloneNotSupportedException {
191186
IBond[] originalBonds = original.getBonds();
192187
IBond[] mappedBonds = mapped.getBonds();
193188

194-
org.hamcrest.MatcherAssert.assertThat("first bond was te same as the original", mappedBonds[0],
189+
org.hamcrest.MatcherAssert.assertThat("first bond was the same as the original", mappedBonds[0],
195190
is(not(sameInstance(originalBonds[0]))));
196-
org.hamcrest.MatcherAssert.assertThat("first mapped bond was not the clone", mappedBonds[0], is(sameInstance(c1o3clone)));
197-
org.hamcrest.MatcherAssert.assertThat("second bond was te same as the original", mappedBonds[1],
191+
org.hamcrest.MatcherAssert.assertThat("first mapped bond was not the clone", mappedBonds[0], is(sameInstance(c2o4clone)));
192+
org.hamcrest.MatcherAssert.assertThat("second bond was the same as the original", mappedBonds[1],
198193
is(not(sameInstance(originalBonds[1]))));
199-
org.hamcrest.MatcherAssert.assertThat("second mapped bond was not the clone", mappedBonds[1], is(sameInstance(c2o4clone)));
194+
org.hamcrest.MatcherAssert.assertThat("second mapped bond was not the clone", mappedBonds[1], is(sameInstance(c1o3clone)));
200195

201196
org.hamcrest.MatcherAssert.assertThat("stereo was not mapped", mapped.getStereo(), is(original.getStereo()));
202197

203198
}
204199

205200
@Test
206-
void testMap_Null_Map() throws CloneNotSupportedException {
201+
void testMap_Null_Map() {
207202
Assertions.assertThrows(IllegalArgumentException.class,
208203
() -> {
209204
IChemObjectBuilder builder = DefaultChemObjectBuilder.getInstance();
@@ -222,12 +217,12 @@ void testMap_Null_Map() throws CloneNotSupportedException {
222217
Conformation.OPPOSITE);
223218

224219
// map the existing element a new element - should through an IllegalArgumentException
225-
IDoubleBondStereochemistry mapped = original.map(Collections.EMPTY_MAP, null);
220+
IDoubleBondStereochemistry mapped = original.map(Collections.<IAtom,IAtom>emptyMap(), null);
226221
});
227222
}
228223

229224
@Test
230-
void testMap_Map_Map_EmptyMapping() throws CloneNotSupportedException {
225+
void testMap_Map_Map_EmptyMapping() {
231226

232227
IChemObjectBuilder builder = DefaultChemObjectBuilder.getInstance();
233228

@@ -245,7 +240,7 @@ void testMap_Map_Map_EmptyMapping() throws CloneNotSupportedException {
245240
Conformation.OPPOSITE);
246241

247242
// map the existing element a new element - should through an IllegalArgumentException
248-
IDoubleBondStereochemistry mapped = original.map(Collections.EMPTY_MAP, Collections.EMPTY_MAP);
243+
IDoubleBondStereochemistry mapped = original.map(Collections.<IAtom,IAtom>emptyMap(), Collections.<IBond,IBond>emptyMap());
249244

250245
org.hamcrest.MatcherAssert.assertThat(mapped, is(sameInstance(original)));
251246
}
@@ -258,4 +253,61 @@ void testToString() {
258253
Assertions.assertNotSame(0, stringRepr.length());
259254
Assertions.assertFalse(stringRepr.contains("\n"));
260255
}
256+
257+
@Test
258+
void testMapStrictCarrierSwapNecessary() throws CDKException {
259+
// arrange
260+
SmilesParser smilesParser = new SmilesParser(SilentChemObjectBuilder.getInstance());
261+
final IAtomContainer molecule1 = smilesParser.parseSmiles("CCC\\C(Cl)=C(/C)Cl");
262+
final IAtomContainer molecule2 = smilesParser.parseSmiles("ClC(C)=C(Cl)CCC");
263+
264+
Map<IChemObject,IChemObject> chemObjectMap = new HashMap<>();
265+
chemObjectMap.put(molecule1.getBond(2), molecule2.getBond(4)); // carrier
266+
chemObjectMap.put(molecule1.getBond(4), molecule2.getBond(2)); // focus
267+
chemObjectMap.put(molecule1.getBond(5), molecule2.getBond(1)); // carrier
268+
IStereoElement<?,?> stereoElement = molecule1.stereoElements().iterator().next();
269+
270+
// act
271+
IStereoElement<?,?> newStereoElement = stereoElement.mapStrict(chemObjectMap);
272+
273+
// assert
274+
DoubleBondStereochemistry newDoubleBondStereochemistry = (DoubleBondStereochemistry) newStereoElement;
275+
Assertions.assertTrue(
276+
newDoubleBondStereochemistry.getFocus().getBegin() == newDoubleBondStereochemistry.getCarriers().get(0).getBegin() ||
277+
newDoubleBondStereochemistry.getFocus().getBegin() == newDoubleBondStereochemistry.getCarriers().get(0).getEnd()
278+
);
279+
Assertions.assertTrue(
280+
newDoubleBondStereochemistry.getFocus().getEnd() == newDoubleBondStereochemistry.getCarriers().get(1).getBegin() ||
281+
newDoubleBondStereochemistry.getFocus().getEnd() == newDoubleBondStereochemistry.getCarriers().get(1).getEnd()
282+
);
283+
}
284+
285+
@Test
286+
void testMapStrictCarrierSwapNotNecessary() throws CDKException {
287+
// arrange
288+
SmilesParser smilesParser = new SmilesParser(SilentChemObjectBuilder.getInstance());
289+
final IAtomContainer molecule1 = smilesParser.parseSmiles("CCC\\C(Cl)=C(/C)Cl");
290+
final IAtomContainer molecule2 = smilesParser.parseSmiles("CCCC(Cl)=C(/C)Cl");
291+
292+
Map<IChemObject,IChemObject> chemObjectMap = new HashMap<>();
293+
chemObjectMap.put(molecule1.getBond(2), molecule2.getBond(2)); // carrier
294+
chemObjectMap.put(molecule1.getBond(4), molecule2.getBond(4)); // focus
295+
chemObjectMap.put(molecule1.getBond(5), molecule2.getBond(5)); // carrier
296+
IStereoElement<?,?> stereoElement = molecule1.stereoElements().iterator().next();
297+
298+
// act
299+
IStereoElement<?,?> newStereoElement = stereoElement.mapStrict(chemObjectMap);
300+
301+
// assert
302+
DoubleBondStereochemistry newDoubleBondStereochemistry = (DoubleBondStereochemistry) newStereoElement;
303+
Assertions.assertTrue(
304+
newDoubleBondStereochemistry.getFocus().getBegin() == newDoubleBondStereochemistry.getCarriers().get(0).getBegin() ||
305+
newDoubleBondStereochemistry.getFocus().getBegin() == newDoubleBondStereochemistry.getCarriers().get(0).getEnd()
306+
);
307+
Assertions.assertTrue(
308+
newDoubleBondStereochemistry.getFocus().getEnd() == newDoubleBondStereochemistry.getCarriers().get(1).getBegin() ||
309+
newDoubleBondStereochemistry.getFocus().getEnd() == newDoubleBondStereochemistry.getCarriers().get(1).getEnd()
310+
);
311+
}
312+
261313
}

‎base/test/src/test/java/org/openscience/cdk/test/interfaces/AbstractAtomContainerTest.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -467,6 +467,7 @@ public void testClone_IStereoElement_DoubleBond() throws Exception {
467467
container.addBond(c2c3);
468468
container.addBond(c1c4);
469469

470+
// note the carrier order is now swapped when it is created
470471
IDoubleBondStereochemistry dbStereo = new DoubleBondStereochemistry(c1c2, new IBond[]{c2c3, c1c4},
471472
IDoubleBondStereochemistry.Conformation.OPPOSITE);
472473

@@ -491,8 +492,8 @@ public void testClone_IStereoElement_DoubleBond() throws Exception {
491492
assertThat("not enough ligands", ligands.length, is(2));
492493

493494
// test same instance - reference equality '=='
494-
assertThat("expected same c2-c3 instance", ligands[0], sameInstance(clone.getBond(1)));
495-
assertThat("expected same c1-c4 instance", ligands[1], sameInstance(clone.getBond(2)));
495+
assertThat("expected same c1-c4 instance", ligands[0], is(clone.getBond(2)));
496+
assertThat("expected same c2-c3 instance", ligands[1], is(clone.getBond(1)));
496497

497498
assertThat("incorrect stereo", clonedDBStereo.getStereo(),
498499
sameInstance(IDoubleBondStereochemistry.Conformation.OPPOSITE));

0 commit comments

Comments
 (0)