Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Reworked AtomContainer.clone() so it is clear what is going on. We no…

…w use a HashMap between the original and cloned atoms to avoid to a linear search each time the atom mapping is needed. This is also useful when we add StereoElement cloning (not yet implemented). We also store a bond mapping as well - we will need the bond mapping for double bond stereo chemistry. The stereo elements in the clone need to be set to an empty array on the clone so we don't remove elements from the original (cloning odditity). It was also clear we need to change the clone() method on Polymer which currently undoes all the cloning work we do in the AtomContainer. For all clone instances I added some code to correctly create HashMaps that won't need to resized. The default HashMap implementation works best at 0.75 capacity - we therefore need to do some simple arithmetic to ensure we don't get a resize. The implemented method is what is used in the Guava library.
  • Loading branch information...
commit 96d6c1af4e38d6259222c299f0ef5160bf2d747e 1 parent 88a2bb2
@johnmay authored
View
155 src/main/org/openscience/cdk/AtomContainer.java
@@ -25,8 +25,10 @@
import java.io.Serializable;
import java.util.ArrayList;
+import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
+import java.util.Map;
import org.openscience.cdk.annotations.TestMethod;
import org.openscience.cdk.interfaces.IAtom;
@@ -1573,73 +1575,92 @@ public String toString()
* @see #shallowCopy
*/
public Object clone() throws CloneNotSupportedException {
- IAtom[] newAtoms;
- IAtomContainer clone = (IAtomContainer) super.clone();
- // start from scratch
- clone.removeAllElements();
- // clone all atoms
- for (int f = 0; f < getAtomCount(); f++) {
- clone.addAtom((IAtom) getAtom(f).clone());
- }
- // clone bonds
- IBond bond;
- IBond newBond;
- for (int i = 0; i < getBondCount(); ++i) {
- bond = getBond(i);
- newBond = (IBond)bond.clone();
- newAtoms = new IAtom[bond.getAtomCount()];
- for (int j = 0; j < bond.getAtomCount(); ++j) {
- newAtoms[j] = clone.getAtom(getAtomNumber(bond.getAtom(j)));
- }
- newBond.setAtoms(newAtoms);
- clone.addBond(newBond);
- }
- ILonePair lp;
- ILonePair newLp;
- for (int i = 0; i < getLonePairCount(); ++i) {
- lp = getLonePair(i);
- newLp = (ILonePair)lp.clone();
- if (lp.getAtom() != null) {
- newLp.setAtom(clone.getAtom(getAtomNumber(lp.getAtom())));
- }
- clone.addLonePair(newLp);
- }
- ISingleElectron se;
- ISingleElectron newSe;
- for (int i = 0; i < getSingleElectronCount(); ++i) {
- se = getSingleElectron(i);
- newSe = (ISingleElectron)se.clone();
- if (se.getAtom() != null) {
- newSe.setAtom(clone.getAtom(getAtomNumber(se.getAtom())));
- }
- clone.addSingleElectron(newSe);
- }
-// for (int f = 0; f < getElectronContainerCount(); f++) {
-// electronContainer = this.getElectronContainer(f);
-// newEC = getBuilder().newElectronContainer();
-// if (electronContainer instanceof IBond) {
-// IBond bond = (IBond) electronContainer;
-// newEC = (IElectronContainer)bond.clone();
-// newAtoms = new IAtom[bond.getAtomCount()];
-// for (int g = 0; g < bond.getAtomCount(); g++) {
-// newAtoms[g] = clone.getAtom(getAtomNumber(bond.getAtom(g)));
-// }
-// ((IBond) newEC).setAtoms(newAtoms);
-// } else if (electronContainer instanceof ILonePair) {
-// IAtom atom = ((ILonePair) electronContainer).getAtom();
-// newEC = (ILonePair)electronContainer.clone();
-// ((ILonePair) newEC).setAtom(clone.getAtom(getAtomNumber(atom)));
-// } else if (electronContainer instanceof ISingleElectron) {
-// IAtom atom = ((ISingleElectron) electronContainer).getAtom();
-// newEC = (ISingleElectron)electronContainer.clone();
-// ((ISingleElectron) newEC).setAtom(clone.getAtom(getAtomNumber(atom)));
-// } else {
-// //logger.debug("Expecting EC, got: " + electronContainer.getClass().getName());
-// newEC = (IElectronContainer) electronContainer.clone();
-// }
-// clone.addElectronContainer(newEC);
-// }
- return clone;
+
+ // this is pretty wasteful as we need to delete most the data
+ // we can't simply create an empty instance as the sub classes (e.g. AminoAcid)
+ // would have a ClassCastException when they invoke clone
+ IAtomContainer clone = (IAtomContainer) super.clone();
+
+ // remove existing elements - we need to set the stereo elements list as list.clone() doesn't
+ // work as expected and will also remove all elements from the original
+ clone.setStereoElements(new ArrayList<IStereoElement>(stereoElements.size()));
+ clone.removeAllElements();
+
+ // create a mapping of the original atoms/bonds to the cloned atoms/bonds
+ // we need this mapping to correctly clone bonds, single/paired electrons
+ // and stereo elements
+ // - the expected size stop the map be resized - method from Google Guava
+ Map<IAtom,IAtom> atomMap = new HashMap<IAtom, IAtom>(atomCount >= 3
+ ? atomCount + atomCount / 3
+ : atomCount + 1);
+ Map<IBond,IBond> bondMap = new HashMap<IBond, IBond>(bondCount >= 3
+ ? bondCount + bondCount / 3
+ : bondCount + 1);
+
+
+
+
+ // clone atoms
+ IAtom[] atoms = new IAtom[this.atomCount];
+ for(int i = 0; i < atoms.length; i++) {
+
+ atoms[i] = (IAtom) this.atoms[i].clone();
+ atomMap.put(this.atoms[i], atoms[i]);
+ }
+ clone.setAtoms(atoms);
+
+
+
+ // clone bonds using a the mappings from the original to the clone
+ IBond[] bonds = new IBond[this.bondCount];
+ for(int i = 0; i < bonds.length; i++){
+
+ IBond original = this.bonds[i];
+ IBond bond = (IBond) original.clone();
+ int n = bond.getAtomCount();
+ IAtom[] members = new IAtom[n];
+
+ for(int j = 0; j < n; j++) {
+ members[j] = atomMap.get(original.getAtom(j));
+ }
+
+ bond.setAtoms(members);
+ bondMap.put(this.bonds[i], bond);
+ bonds[i] = bond;
+ }
+ clone.setBonds(bonds);
+
+
+
+ // clone lone pairs (we can't use an array to buffer as there is no setLonePairs())
+ for (int i = 0; i < lonePairCount; i++) {
+
+ ILonePair original = this.lonePairs[i];
+ ILonePair pair = (ILonePair) original.clone();
+
+ if(pair.getAtom() != null)
+ pair.setAtom(atomMap.get(original.getAtom()));
+
+ clone.addLonePair(pair);
+ }
+
+
+
+ // clone single electrons (we can't use an array to buffer as there is no setSingleElectrons())
+ for (int i = 0; i < singleElectronCount; i++) {
+
+ ISingleElectron original = this.singleElectrons[i];
+ ISingleElectron electron = (ISingleElectron) original.clone();
+
+ if(electron.getAtom() != null)
+ electron.setAtom(atomMap.get(original.getAtom()));
+
+ clone.addSingleElectron(electron);
+ }
+
+
+ return clone;
+
}
/**
View
28 src/main/org/openscience/cdk/Polymer.java
@@ -33,8 +33,11 @@
import org.openscience.cdk.interfaces.IMonomer;
import org.openscience.cdk.interfaces.IPolymer;
import org.openscience.cdk.interfaces.ISingleElectron;
+import org.openscience.cdk.interfaces.IStereoElement;
+import java.util.ArrayList;
import java.util.Collection;
+import java.util.HashMap;
import java.util.Hashtable;
import java.util.Map;
@@ -158,10 +161,24 @@ public Object clone() throws CloneNotSupportedException {
}
}
+ // create a mapping of the original atoms/bonds to the cloned atoms/bonds
+ // we need this mapping to correctly clone bonds, single/paired electrons
+ // and stereo elements
+ // - the expected size stop the map be resized - method from Google Guava
+ Map<IAtom,IAtom> atomMap = new HashMap<IAtom, IAtom>(atomCount >= 3
+ ? atomCount + atomCount / 3
+ : atomCount + 1);
+ Map<IBond,IBond> bondMap = new HashMap<IBond, IBond>(bondCount >= 3
+ ? bondCount + bondCount / 3
+ : bondCount + 1);
+
// now consider atoms that are not associated with any monomer
for (IAtom atom : atoms()) {
- if (!atomIsInMonomer(atom))
- clone.addAtom((IAtom) atom.clone());
+ if (!atomIsInMonomer(atom)) {
+ IAtom cloned = (IAtom) atom.clone();
+ clone.addAtom(cloned);
+ atomMap.put(atom, cloned);
+ }
}
// since we already removed bonds we'll have to add them back
@@ -170,10 +187,11 @@ public Object clone() throws CloneNotSupportedException {
newBond = (IBond)bond.clone();
IAtom[] newAtoms = new IAtom[bond.getAtomCount()];
for (int j = 0; j < bond.getAtomCount(); ++j) {
- newAtoms[j] = clone.getAtom(getAtomNumber(bond.getAtom(j)));
+ newAtoms[j] = atomMap.get(bond.getAtom(j));
}
newBond.setAtoms(newAtoms);
clone.addBond(newBond);
+ bondMap.put(bond, newBond);
}
// put back lone pairs
@@ -183,7 +201,7 @@ public Object clone() throws CloneNotSupportedException {
lp = getLonePair(i);
newLp = (ILonePair) lp.clone();
if (lp.getAtom() != null) {
- newLp.setAtom(clone.getAtom(getAtomNumber(lp.getAtom())));
+ newLp.setAtom(atomMap.get(lp.getAtom()));
}
clone.addLonePair(newLp);
}
@@ -195,7 +213,7 @@ public Object clone() throws CloneNotSupportedException {
singleElectron = getSingleElectron(i);
newSingleElectron = (ISingleElectron) singleElectron.clone();
if (singleElectron.getAtom() != null) {
- newSingleElectron.setAtom(clone.getAtom(getAtomNumber(singleElectron.getAtom())));
+ newSingleElectron.setAtom(atomMap.get(singleElectron.getAtom()));
}
clone.addSingleElectron(newSingleElectron);
}
View
158 src/main/org/openscience/cdk/silent/AtomContainer.java
@@ -20,8 +20,10 @@
import java.io.Serializable;
import java.util.ArrayList;
+import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
+import java.util.Map;
import org.openscience.cdk.annotations.TestMethod;
import org.openscience.cdk.interfaces.IAtom;
@@ -1516,74 +1518,94 @@ public String toString()
* @see #shallowCopy
*/
public Object clone() throws CloneNotSupportedException {
- IAtom[] newAtoms;
- IAtomContainer clone = (IAtomContainer) super.clone();
- // start from scratch
- clone.removeAllElements();
- // clone all atoms
- for (int f = 0; f < getAtomCount(); f++) {
- clone.addAtom((IAtom) getAtom(f).clone());
- }
- // clone bonds
- IBond bond;
- IBond newBond;
- for (int i = 0; i < getBondCount(); ++i) {
- bond = getBond(i);
- newBond = (IBond)bond.clone();
- newAtoms = new IAtom[bond.getAtomCount()];
- for (int j = 0; j < bond.getAtomCount(); ++j) {
- newAtoms[j] = clone.getAtom(getAtomNumber(bond.getAtom(j)));
- }
- newBond.setAtoms(newAtoms);
- clone.addBond(newBond);
- }
- ILonePair lp;
- ILonePair newLp;
- for (int i = 0; i < getLonePairCount(); ++i) {
- lp = getLonePair(i);
- newLp = (ILonePair)lp.clone();
- if (lp.getAtom() != null) {
- newLp.setAtom(clone.getAtom(getAtomNumber(lp.getAtom())));
- }
- clone.addLonePair(newLp);
- }
- ISingleElectron se;
- ISingleElectron newSe;
- for (int i = 0; i < getSingleElectronCount(); ++i) {
- se = getSingleElectron(i);
- newSe = (ISingleElectron)se.clone();
- if (se.getAtom() != null) {
- newSe.setAtom(clone.getAtom(getAtomNumber(se.getAtom())));
- }
- clone.addSingleElectron(newSe);
- }
-// for (int f = 0; f < getElectronContainerCount(); f++) {
-// electronContainer = this.getElectronContainer(f);
-// newEC = getBuilder().newElectronContainer();
-// if (electronContainer instanceof IBond) {
-// IBond bond = (IBond) electronContainer;
-// newEC = (IElectronContainer)bond.clone();
-// newAtoms = new IAtom[bond.getAtomCount()];
-// for (int g = 0; g < bond.getAtomCount(); g++) {
-// newAtoms[g] = clone.getAtom(getAtomNumber(bond.getAtom(g)));
-// }
-// ((IBond) newEC).setAtoms(newAtoms);
-// } else if (electronContainer instanceof ILonePair) {
-// IAtom atom = ((ILonePair) electronContainer).getAtom();
-// newEC = (ILonePair)electronContainer.clone();
-// ((ILonePair) newEC).setAtom(clone.getAtom(getAtomNumber(atom)));
-// } else if (electronContainer instanceof ISingleElectron) {
-// IAtom atom = ((ISingleElectron) electronContainer).getAtom();
-// newEC = (ISingleElectron)electronContainer.clone();
-// ((ISingleElectron) newEC).setAtom(clone.getAtom(getAtomNumber(atom)));
-// } else {
-// //logger.debug("Expecting EC, got: " + electronContainer.getClass().getName());
-// newEC = (IElectronContainer) electronContainer.clone();
-// }
-// clone.addElectronContainer(newEC);
-// }
- return clone;
- }
+
+
+ // this is pretty wasteful as we need to delete most the data
+ // we can't simply create an empty instance as the sub classes (e.g. AminoAcid)
+ // would have a ClassCastException when they invoke clone
+ IAtomContainer clone = (IAtomContainer) super.clone();
+
+ // remove existing elements - we need to set the stereo elements list as list.clone() doesn't
+ // work as expected and will also remove all elements from the original
+ clone.setStereoElements(new ArrayList<IStereoElement>(stereoElements.size()));
+ clone.removeAllElements();
+
+ // create a mapping of the original atoms/bonds to the cloned atoms/bonds
+ // we need this mapping to correctly clone bonds, single/paired electrons
+ // and stereo elements
+ // - the expected size stop the map be resized - method from Google Guava
+ Map<IAtom,IAtom> atomMap = new HashMap<IAtom, IAtom>(atomCount >= 3
+ ? atomCount + atomCount / 3
+ : atomCount + 1);
+ Map<IBond,IBond> bondMap = new HashMap<IBond, IBond>(bondCount >= 3
+ ? bondCount + bondCount / 3
+ : bondCount + 1);
+
+
+
+ // clone atoms
+ IAtom[] atoms = new IAtom[this.atomCount];
+ for(int i = 0; i < atoms.length; i++) {
+
+ atoms[i] = (IAtom) this.atoms[i].clone();
+ atomMap.put(this.atoms[i], atoms[i]);
+ }
+ clone.setAtoms(atoms);
+
+
+
+ // clone bonds using a the mappings from the original to the clone
+ IBond[] bonds = new IBond[this.bondCount];
+ for(int i = 0; i < bonds.length; i++){
+
+ IBond original = this.bonds[i];
+ IBond bond = (IBond) original.clone();
+ int n = bond.getAtomCount();
+ IAtom[] members = new IAtom[n];
+
+ for(int j = 0; j < n; j++) {
+ members[j] = atomMap.get(original.getAtom(j));
+ }
+
+ bond.setAtoms(members);
+ bondMap.put(this.bonds[i], bond);
+ bonds[i] = bond;
+ }
+ clone.setBonds(bonds);
+
+
+
+ // clone lone pairs (we can't use an array to buffer as there is no setLonePairs())
+ for (int i = 0; i < lonePairCount; i++) {
+
+ ILonePair original = this.lonePairs[i];
+ ILonePair pair = (ILonePair) original.clone();
+
+ if(pair.getAtom() != null)
+ pair.setAtom(atomMap.get(original.getAtom()));
+
+ clone.addLonePair(pair);
+ }
+
+
+
+ // clone single electrons (we can't use an array to buffer as there is no setSingleElectrons())
+ for (int i = 0; i < singleElectronCount; i++) {
+
+ ISingleElectron original = this.singleElectrons[i];
+ ISingleElectron electron = (ISingleElectron) original.clone();
+
+ if(electron.getAtom() != null)
+ electron.setAtom(atomMap.get(original.getAtom()));
+
+ clone.addSingleElectron(electron);
+ }
+
+
+ return clone;
+
+
+ }
/**
* Grows the ElectronContainer array by a given size.
View
41 src/main/org/openscience/cdk/silent/Polymer.java
@@ -28,8 +28,10 @@
import org.openscience.cdk.interfaces.IMonomer;
import org.openscience.cdk.interfaces.IPolymer;
import org.openscience.cdk.interfaces.ISingleElectron;
+import org.openscience.cdk.interfaces.IStereoElement;
import java.util.Collection;
+import java.util.HashMap;
import java.util.Hashtable;
import java.util.Map;
@@ -153,22 +155,37 @@ public Object clone() throws CloneNotSupportedException {
}
}
+ // create a mapping of the original atoms/bonds to the cloned atoms/bonds
+ // we need this mapping to correctly clone bonds, single/paired electrons
+ // and stereo elements
+ // - the expected size stop the map be resized - method from Google Guava
+ Map<IAtom,IAtom> atomMap = new HashMap<IAtom, IAtom>(atomCount >= 3
+ ? atomCount + atomCount / 3
+ : atomCount + 1);
+ Map<IBond,IBond> bondMap = new HashMap<IBond, IBond>(bondCount >= 3
+ ? bondCount + bondCount / 3
+ : bondCount + 1);
+
// now consider atoms that are not associated with any monomer
for (IAtom atom : atoms()) {
- if (!atomIsInMonomer(atom))
- clone.addAtom((IAtom) atom.clone());
+ if (!atomIsInMonomer(atom)) {
+ IAtom cloned = (IAtom) atom.clone();
+ clone.addAtom(cloned);
+ atomMap.put(atom, cloned);
+ }
}
// since we already removed bonds we'll have to add them back
- IBond newBond;
- for (IBond bond : bonds()) {
- newBond = (IBond)bond.clone();
- IAtom[] newAtoms = new IAtom[bond.getAtomCount()];
- for (int j = 0; j < bond.getAtomCount(); ++j) {
- newAtoms[j] = clone.getAtom(getAtomNumber(bond.getAtom(j)));
- }
- newBond.setAtoms(newAtoms);
+ IBond newBond;
+ for (IBond bond : bonds()) {
+ newBond = (IBond)bond.clone();
+ IAtom[] newAtoms = new IAtom[bond.getAtomCount()];
+ for (int j = 0; j < bond.getAtomCount(); ++j) {
+ newAtoms[j] = atomMap.get(bond.getAtom(j));
+ }
+ newBond.setAtoms(newAtoms);
clone.addBond(newBond);
+ bondMap.put(bond, newBond);
}
// put back lone pairs
@@ -178,7 +195,7 @@ public Object clone() throws CloneNotSupportedException {
lp = getLonePair(i);
newLp = (ILonePair) lp.clone();
if (lp.getAtom() != null) {
- newLp.setAtom(clone.getAtom(getAtomNumber(lp.getAtom())));
+ newLp.setAtom(atomMap.get(lp.getAtom()));
}
clone.addLonePair(newLp);
}
@@ -190,7 +207,7 @@ public Object clone() throws CloneNotSupportedException {
singleElectron = getSingleElectron(i);
newSingleElectron = (ISingleElectron) singleElectron.clone();
if (singleElectron.getAtom() != null) {
- newSingleElectron.setAtom(clone.getAtom(getAtomNumber(singleElectron.getAtom())));
+ newSingleElectron.setAtom(atomMap.get(singleElectron.getAtom()));
}
clone.addSingleElectron(newSingleElectron);
}
Please sign in to comment.
Something went wrong with that request. Please try again.