Skip to content

Commit

Permalink
Better state management for stereochemistry. When an stereo element i…
Browse files Browse the repository at this point in the history 
…s added to a molecule it's atom/bond components are remapped to the internal AtomRef/BondRef of the molecule in question. This ensure any future actions such as swapping and atom for another work correctly. This is now more robust and we get an error if we try to add stereochemistry that includes atom/bonds not in this molecule - this makes sense.
  • Loading branch information
@johnmay @egonw
johnmay authored and egonw committed Feb 3, 2022
1 parent 1078174 commit 7fb6341
Show file tree
Hide file tree
Showing 5 changed files with 184 additions and 37 deletions.
50 changes: 36 additions & 14 deletions base/data/src/main/java/org/openscience/cdk/AtomContainer2.java
View file
Expand Up @@ -23,6 +23,7 @@
package org.openscience.cdk;

import org.openscience.cdk.exception.NoSuchAtomException;
import org.openscience.cdk.exception.NoSuchBondException;
import org.openscience.cdk.interfaces.IAtom;
import org.openscience.cdk.interfaces.IAtomContainer;
import org.openscience.cdk.interfaces.IBond;
Expand Down Expand Up @@ -213,6 +214,23 @@ private BondRef getBondRefUnsafe(IBond bond) {
return null;
}


private BondRef getBondRef(IBond bond) {
BondRef ref = getBondRefUnsafe(bond);
if (ref == null)
throw new NoSuchBondException("Atom is not a member of this AtomContainer");
return ref;
}

private IChemObject getRef(IChemObject cobj) {
if (cobj instanceof IAtom)
return getAtomRef((IAtom)cobj);
else if (cobj instanceof IBond)
return getBondRef((IBond)cobj);
else
throw new IllegalArgumentException("Atom or Bond must be provided");
}

private BaseBondRef newBondRef(IBond bond) {
BaseAtomRef beg = bond.getBegin() == null ? null : getAtomRef(bond.getBegin());
BaseAtomRef end = bond.getEnd() == null ? null : getAtomRef(bond.getEnd());
Expand Down Expand Up @@ -251,7 +269,12 @@ private void delFromEndpoints(BondRef bondref) {
*/
@Override
public void addStereoElement(IStereoElement element) {
stereo.add(element);
// important! we need to re-wrap any Atom/Bond refs with "our" own ones
Map<IChemObject,IChemObject> remap = new HashMap<>();
remap.put(element.getFocus(), getRef(element.getFocus()));
for (Object cobj : element.getCarriers())
remap.put((IChemObject) cobj, getRef((IChemObject)cobj));
stereo.add(element.map(remap));
notifyChanged();
}

Expand All @@ -261,7 +284,9 @@ public void addStereoElement(IStereoElement element) {
@Override
public void setStereoElements(List<IStereoElement> elements) {
this.stereo.clear();
this.stereo.addAll(elements);
for (IStereoElement se : elements) {
addStereoElement(se);
}
notifyChanged();
}

Expand Down Expand Up @@ -859,18 +884,11 @@ public void add(IAtomContainer that) {
for (IBond bond : this.bonds())
bond.setFlag(CDKConstants.VISITED, true);

// do stereo elements first
for (IStereoElement se : that.stereoElements()) {
if (se instanceof TetrahedralChirality &&
!((TetrahedralChirality) se).getChiralAtom().getFlag(CDKConstants.VISITED)) {
this.addStereoElement(se);
} else if (se instanceof DoubleBondStereochemistry &&
!((DoubleBondStereochemistry) se).getStereoBond().getFlag(CDKConstants.VISITED)) {
this.addStereoElement(se);
} else if (se instanceof ExtendedTetrahedral &&
!((ExtendedTetrahedral) se).focus().getFlag(CDKConstants.VISITED)) {
this.addStereoElement(se);
}
// Determine which stereo elements are new (unvisited)
List<IStereoElement<?,?>> newStereo = new ArrayList<>();
for (IStereoElement<?,?> stereo : that.stereoElements()) {
if (!stereo.getFocus().getFlag(CDKConstants.VISITED))
newStereo.add(stereo);
}

// append atoms/bonds not visited
Expand All @@ -897,6 +915,10 @@ public void add(IAtomContainer that) {
if (this.indexOf(lp) < 0)
addLonePair(lp);
}
// now add the new stereo elements last (needs atom/bond remap)
for (IStereoElement<?,?> se : newStereo) {
this.addStereoElement(se);
}

notifyChanged();
}
Expand Down
43 changes: 43 additions & 0 deletions base/interfaces/src/main/java/org/openscience/cdk/exception/NoSuchBondException.java
View file
@@ -0,0 +1,43 @@
/* Copyright (C) 2001-2007 The Chemistry Development Kit (CKD) project
*
* Contact: cdk-devel@lists.sourceforge.net
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public License
* as published by the Free Software Foundation; either version 2.1
* of the License, or (at your option) any later version.
* All I ask is that proper credit is given for my work, which includes
* - but is not limited to - adding the above copyright notice to the beginning
* of your source code files, and to any copyright notice that you may distribute
* with programs based on this work.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
*/
package org.openscience.cdk.exception;

/**
* Exception that is thrown when a Bond is requested or required that
* does not exist in the relevant environment.
*
* @cdk.module core
* @cdk.githash
*/
public class NoSuchBondException extends RuntimeException {

/**
* Constructs a new NoSuchBondException with the given message.
*
* @param message for the constructed exception
*/
public NoSuchBondException(String message) {
super(message);
}
}
51 changes: 37 additions & 14 deletions base/silent/src/main/java/org/openscience/cdk/silent/AtomContainer2.java
View file
Expand Up @@ -24,6 +24,7 @@

import org.openscience.cdk.CDKConstants;
import org.openscience.cdk.exception.NoSuchAtomException;
import org.openscience.cdk.exception.NoSuchBondException;
import org.openscience.cdk.interfaces.IAtom;
import org.openscience.cdk.interfaces.IAtomContainer;
import org.openscience.cdk.interfaces.IBond;
Expand Down Expand Up @@ -216,6 +217,23 @@ private BondRef getBondRefUnsafe(IBond bond) {
return null;
}

private BondRef getBondRef(IBond bond) {
BondRef ref = getBondRefUnsafe(bond);
if (ref == null)
throw new NoSuchBondException("Atom is not a member of this AtomContainer");
return ref;
}

// Obtain the AtomRef or BondRef for a ChemObject
private IChemObject getRef(IChemObject cobj) {
if (cobj instanceof IAtom)
return getAtomRef((IAtom)cobj);
else if (cobj instanceof IBond)
return getBondRef((IBond)cobj);
else
throw new IllegalArgumentException("Atom or Bond must be provided");
}

private BaseBondRef newBondRef(IBond bond) {
BaseAtomRef beg = bond.getBegin() == null ? null : getAtomRef(bond.getBegin());
BaseAtomRef end = bond.getEnd() == null ? null : getAtomRef(bond.getEnd());
Expand Down Expand Up @@ -254,7 +272,12 @@ private void delFromEndpoints(BondRef bondref) {
*/
@Override
public void addStereoElement(IStereoElement element) {
stereo.add(element);
// important! we need to re-wrap any Atom/Bond refs with "our" own ones
Map<IChemObject,IChemObject> remap = new HashMap<>();
remap.put(element.getFocus(), getRef(element.getFocus()));
for (Object cobj : element.getCarriers())
remap.put((IChemObject) cobj, getRef((IChemObject)cobj));
stereo.add(element.map(remap));
}

/**
Expand All @@ -263,7 +286,9 @@ public void addStereoElement(IStereoElement element) {
@Override
public void setStereoElements(List<IStereoElement> elements) {
this.stereo.clear();
this.stereo.addAll(elements);
for (IStereoElement se : elements) {
addStereoElement(se);
}
}

/**
Expand Down Expand Up @@ -847,18 +872,11 @@ public void add(IAtomContainer that) {
for (IBond bond : this.bonds())
bond.setFlag(CDKConstants.VISITED, true);

// do stereo elements first
for (IStereoElement se : that.stereoElements()) {
if (se instanceof TetrahedralChirality &&
!((TetrahedralChirality) se).getChiralAtom().getFlag(CDKConstants.VISITED)) {
this.addStereoElement(se);
} else if (se instanceof DoubleBondStereochemistry &&
!((DoubleBondStereochemistry) se).getStereoBond().getFlag(CDKConstants.VISITED)) {
this.addStereoElement(se);
} else if (se instanceof ExtendedTetrahedral &&
!((ExtendedTetrahedral) se).focus().getFlag(CDKConstants.VISITED)) {
this.addStereoElement(se);
}
// Determine which stereo elements are new (unvisited)
List<IStereoElement<?,?>> newStereo = new ArrayList<>();
for (IStereoElement<?,?> stereo : that.stereoElements()) {
if (!stereo.getFocus().getFlag(CDKConstants.VISITED))
newStereo.add(stereo);
}

// append atoms/bonds not visited
Expand All @@ -884,6 +902,11 @@ public void add(IAtomContainer that) {
if (this.indexOf(lp) < 0)
addLonePair(lp);
}

// now add the new stereo elements last (needs atom/bond remap)
for (IStereoElement<?,?> se : newStereo) {
this.addStereoElement(se);
}
}

/**
Expand Down
39 changes: 30 additions & 9 deletions base/test/src/main/java/org/openscience/cdk/test/interfaces/AbstractAtomContainerTest.java
View file
Expand Up @@ -596,26 +596,44 @@ public void testSetStereoElements_List() {
IBond b1 = container.getBuilder().newBond();
IBond b2 = container.getBuilder().newBond();

org.hamcrest.MatcherAssert.assertThat("empty container had stereo elements", container.stereoElements().iterator().hasNext(),
assertThat("empty container had stereo elements", container.stereoElements().iterator().hasNext(),
is(false));

List<IStereoElement> dbElements = new ArrayList<IStereoElement>();
dbElements.add(new DoubleBondStereochemistry(bond, new IBond[]{b1, b2},
IDoubleBondStereochemistry.Conformation.TOGETHER));
container.setAtoms(new IAtom[]{atom, a1, a2, a3, a4});
container.setBonds(new IBond[]{bond, b1, b2});
container.setStereoElements(dbElements);
Iterator<IStereoElement> first = container.stereoElements().iterator();
org.hamcrest.MatcherAssert.assertThat("container did not have stereo elements", first.hasNext(), is(true));
org.hamcrest.MatcherAssert.assertThat("expected element to equal set element (double bond)", first.next(), is(dbElements.get(0)));
org.hamcrest.MatcherAssert.assertThat("container had more then one stereo element", first.hasNext(), is(false));
assertThat("container did not have stereo elements", first.hasNext(), is(true));
IStereoElement<IBond,IBond> dbActual = first.next();
assertThat("expected element to equal set element (double bond)",
dbActual.getConfig(),
is(dbElements.get(0).getConfig()));
assertThat("expected db foucs was wrong",
dbActual.getFocus(),
is(dbElements.get(0).getFocus()));
assertThat("expected db carriers were wrong",
dbActual.getCarriers(),
is(dbElements.get(0).getCarriers()));
assertThat("container had more then one stereo element", first.hasNext(), is(false));

List<IStereoElement> tetrahedralElements = new ArrayList<IStereoElement>();
tetrahedralElements.add(new TetrahedralChirality(atom, new IAtom[]{a1, a2, a3, a4}, ITetrahedralChirality.Stereo.CLOCKWISE));
container.setStereoElements(tetrahedralElements);
Iterator<IStereoElement> second = container.stereoElements().iterator();
org.hamcrest.MatcherAssert.assertThat("container did not have stereo elements", second.hasNext(), is(true));
org.hamcrest.MatcherAssert.assertThat("expected element to equal set element (tetrahedral)", second.next(),
is(tetrahedralElements.get(0)));
org.hamcrest.MatcherAssert.assertThat("container had more then one stereo element", second.hasNext(), is(false));
assertThat("container did not have stereo elements", second.hasNext(), is(true));
IStereoElement<IAtom,IAtom> thActual = second.next();
assertThat("expected element to equal set element (tetrahedral)", thActual.getConfig(),
is(tetrahedralElements.get(0).getConfig()));
assertThat("expected db foucs was wrong",
thActual.getFocus(),
is(tetrahedralElements.get(0).getFocus()));
assertThat("expected db carriers were wrong",
thActual.getCarriers(),
is(tetrahedralElements.get(0).getCarriers()));
assertThat("container had more then one stereo element", second.hasNext(), is(false));

}

Expand Down Expand Up @@ -922,6 +940,7 @@ public void testRemoveAllElements_StereoElements() {
IAtom a2 = builder.newAtom();
IAtom a3 = builder.newAtom();
IAtom a4 = builder.newAtom();
container.setAtoms(new IAtom[]{focus, a1, a2, a3, a4});
container.addStereoElement(new TetrahedralChirality(focus,
new IAtom[]{a1,a2,a3,a4},
ITetrahedralChirality.Stereo.CLOCKWISE));
Expand Down Expand Up @@ -2514,8 +2533,10 @@ public void testStereoElements() {
IAtom carbon4 = container.getBuilder().newInstance(IAtom.class, "C");
carbon4.setID("c4");
int parityInt = 1;
container.setAtoms(new IAtom[]{carbon, carbon1, carbon2, carbon3, carbon4});
IStereoElement stereoElement = container.getBuilder().newInstance(ITetrahedralChirality.class, carbon,
new IAtom[]{carbon1, carbon2, carbon3, carbon4}, ITetrahedralChirality.Stereo.CLOCKWISE);
container.setAtoms(new IAtom[]{carbon,carbon1,carbon2,carbon3,carbon4});
container.addStereoElement(stereoElement);

Iterator<IStereoElement> stereoElements = container.stereoElements().iterator();
Expand Down Expand Up @@ -3007,7 +3028,7 @@ public void testIsEmpty() throws Exception {
container.removeAtomOnly(c1);
container.removeAtomOnly(c2);

org.hamcrest.MatcherAssert.assertThat("atom contains contains no bonds", container.getBondCount(), CoreMatchers.is(1));
assertThat("atom contains contains no bonds", container.getBondCount(), CoreMatchers.is(1));

assertTrue("atom contains contains no atoms but was not empty", container.isEmpty());

Expand Down
38 changes: 38 additions & 0 deletions storage/smiles/src/test/java/org/openscience/cdk/smiles/SmilesGeneratorTest.java
View file
Expand Up @@ -23,6 +23,7 @@
import org.junit.experimental.categories.Category;
import org.openscience.cdk.Atom;
import org.openscience.cdk.AtomContainer;
import org.openscience.cdk.exception.InvalidSmilesException;
import org.openscience.cdk.test.CDKTestCase;
import org.openscience.cdk.ChemFile;
import org.openscience.cdk.DefaultChemObjectBuilder;
Expand Down Expand Up @@ -63,6 +64,7 @@
import org.openscience.cdk.tools.CDKHydrogenAdder;
import org.openscience.cdk.tools.manipulator.AtomContainerManipulator;
import org.openscience.cdk.tools.manipulator.ChemFileManipulator;
import org.openscience.cdk.tools.manipulator.ReactionManipulator;

import javax.vecmath.Point2d;
import java.io.ByteArrayInputStream;
Expand Down Expand Up @@ -1301,6 +1303,42 @@ public void canonAtomMapsRenumber() throws CDKException {
is("[*:1]CCC([*:2])[*:3]"));
}

@Test
public void stereoElementRemap_Silent() throws CDKException {
SmilesParser smipar = new SmilesParser(SilentChemObjectBuilder.getInstance());
IReaction rxn = smipar.parseReactionSmiles("*/C=C/C>> |$R$|");
String res = null;
for (IAtomContainer reactant : ReactionManipulator.getAllReactants(rxn).atomContainers())
res = new SmilesGenerator(SmiFlavor.Default).create(reactant);
Assert.assertEquals("*/C=C/C |$R$|", res);
}

@Test
public void stereoElementRemap_Default() throws CDKException {
SmilesParser smipar = new SmilesParser(DefaultChemObjectBuilder.getInstance());
IReaction rxn = smipar.parseReactionSmiles("*/C=C/C>> |$R$|");
String res = null;
for (IAtomContainer reactant : ReactionManipulator.getAllReactants(rxn).atomContainers())
res = new SmilesGenerator(SmiFlavor.Default).create(reactant);
Assert.assertEquals("*/C=C/C |$R$|", res);
}

@Test
public void stereoElementRemapRxn_Silent() throws CDKException {
SmilesParser smipar = new SmilesParser(SilentChemObjectBuilder.getInstance());
IReaction rxn = smipar.parseReactionSmiles("*/C=C/C>> |$R$|");
String res = null;
Assert.assertEquals("*/C=C/C>> |$R$|", new SmilesGenerator(SmiFlavor.Default).create(rxn));
}

@Test
public void stereoElementRemapRxn_Default() throws CDKException {
SmilesParser smipar = new SmilesParser(DefaultChemObjectBuilder.getInstance());
IReaction rxn = smipar.parseReactionSmiles("*/C=C/C>> |$R$|");
String res = null;
Assert.assertEquals("*/C=C/C>> |$R$|", new SmilesGenerator(SmiFlavor.Default).create(rxn));
}

static ITetrahedralChirality anticlockwise(IAtomContainer container, int central, int a1, int a2, int a3, int a4) {
return new TetrahedralChirality(container.getAtom(central), new IAtom[]{container.getAtom(a1),
container.getAtom(a2), container.getAtom(a3), container.getAtom(a4)},
Expand Down

0 comments on commit 7fb6341

Please sign in to comment.