Skip to content

Commit

Permalink
updated SMSD code Signed-off-by: Syed Asad Rahman <s9asad@gmail.com>
Browse files Browse the repository at this point in the history
Signed-off-by: Rajarshi  Guha <rajarshi.guha@gmail.com>
  • Loading branch information
asad authored and rajarshi committed Jun 26, 2010
1 parent 935b5f6 commit 370a926
Show file tree
Hide file tree
Showing 19 changed files with 206 additions and 411 deletions.
Expand Up @@ -30,7 +30,7 @@
import java.util.TreeMap;
import org.openscience.cdk.interfaces.IMolecule;
import org.openscience.cdk.smsd.helper.FinalMappings;
import org.openscience.cdk.smsd.helper.MolHandler;
import org.openscience.cdk.smsd.tools.MolHandler;
import org.openscience.cdk.annotations.TestClass;
import org.openscience.cdk.annotations.TestMethod;
import org.openscience.cdk.exception.CDKException;
Expand Down
Expand Up @@ -52,7 +52,7 @@ private static List<Integer> extractCliqueMapping(List<Integer> comp_graph_nodes
List<Integer> clique_List = new ArrayList<Integer>(clique_List_org);
int clique_siz = clique_List.size();
int vec_size = comp_graph_nodes.size();
//System.out.println("VEC SIZE " + vec_size);
// System.out.println("VEC SIZE " + vec_size);

This comment has been minimized.

Copy link
@egonw

egonw Jun 26, 2010

Member

Consider using the ILoggingTool functionality:

logger.debug"(VEC SIZE ", vec_size);

This comment has been minimized.

Copy link
@asad

asad Jun 26, 2010

Author Contributor

sure will keep note that for the next update!

for (int a = 0; a < clique_siz; a++) {
for (int b = 0; b < vec_size; b += 3) {
if (clique_List.get(a) == comp_graph_nodes.get(b + 2)) {
Expand All @@ -77,10 +77,8 @@ protected static List<List<Integer>> extractMapping(List<List<Integer>> _mapping
List<Integer> clique_List_org) {

try {

List<Integer> clique_List = extractCliqueMapping(comp_graph_nodes, clique_List_org);
_mappings.add(clique_List);

} catch (Exception e) {
System.err.println("Error in FinalMapping List: " + e.getCause());
e.printStackTrace();
Expand Down
Expand Up @@ -329,8 +329,6 @@ protected int compatibilityGraphCEdgeZero() throws IOException {
//Size of C and D edges of the compatibility graph
cEdgesSize = cEdges.size();
dEdgesSize = dEdges.size();


return 0;
}

Expand Down
Expand Up @@ -37,7 +37,7 @@
import org.openscience.cdk.interfaces.IMolecule;
import org.openscience.cdk.smsd.filters.PostFilter;
import org.openscience.cdk.smsd.helper.FinalMappings;
import org.openscience.cdk.smsd.helper.MolHandler;
import org.openscience.cdk.smsd.tools.MolHandler;
import org.openscience.cdk.smsd.interfaces.AbstractMCSAlgorithm;
import org.openscience.cdk.smsd.interfaces.IMCSBase;

Expand Down Expand Up @@ -94,6 +94,7 @@ public void set(IAtomContainer source, IAtomContainer target) {
* @param target
*/
@TestMethod("testSet_IMolecule_IMolecule")
@Override

This comment has been minimized.

Copy link
@egonw

egonw Jun 26, 2010

Member

This could have been in a separate commit too.

public void set(IMolecule source, IMolecule target) throws CDKException {

IMolecule mol1 = source;
Expand All @@ -120,8 +121,6 @@ public void set(String sourceMolFileName, String targetMolFileName) {
MolHandler Reactant = new MolHandler(mol1, false);
MolHandler Product = new MolHandler(mol2, false);
set(Reactant, Product);


}

/** {@inheritDoc}
Expand Down Expand Up @@ -150,9 +149,7 @@ public void searchMCS() {
} else {
flagExchange = true;
mappings = new MCSPlus().getOverlaps(target, source);

}

PostFilter.filter(mappings);
setAllMapping();
setAllAtomMapping();
Expand All @@ -170,8 +167,7 @@ private final void setAllMapping() {
int counter = 0;
for (Map<Integer, Integer> solution : final_solution) {
// System.out.println("Number of MCS solution: " + solution);
TreeMap<Integer, Integer> validSolution = new TreeMap<Integer, Integer>();

Map<Integer, Integer> validSolution = new TreeMap<Integer, Integer>();
if (!flagExchange) {
for (Map.Entry<Integer, Integer> map : solution.entrySet()) {
validSolution.put(map.getKey(), map.getValue());
Expand All @@ -193,34 +189,29 @@ private final void setAllMapping() {
private final synchronized void setAllAtomMapping() {

try {
List<Map<Integer, Integer>> final_solution = FinalMappings.getInstance().getFinalMapping();

int counter = 0;
for (Map<Integer, Integer> solution : final_solution) {


for (Map<Integer, Integer> solution : allMCS) {
Map<IAtom, IAtom> atomMappings = new HashMap<IAtom, IAtom>();

for (Map.Entry<Integer, Integer> map : solution.entrySet()) {

int IIndex = map.getKey();
int JIndex = map.getValue();


IAtom sourceAtom = null;
IAtom targetAtom = null;

if (!flagExchange) {
sourceAtom = source.getAtom(IIndex);
targetAtom = target.getAtom(JIndex);
} else {
sourceAtom = source.getAtom(JIndex);
targetAtom = target.getAtom(IIndex);
}

//
// if (!flagExchange) {
// sourceAtom = source.getAtom(IIndex);
// targetAtom = target.getAtom(JIndex);
// } else {
// sourceAtom = source.getAtom(JIndex);
// targetAtom = target.getAtom(IIndex);
// }
sourceAtom = source.getAtom(IIndex);
targetAtom = target.getAtom(JIndex);
atomMappings.put(sourceAtom, targetAtom);
}

allAtomMCS.add(counter++, atomMappings);
}
} catch (Exception I) {
Expand All @@ -231,15 +222,14 @@ private final synchronized void setAllAtomMapping() {

private synchronized void setFirstMapping() {
if (!allMCS.isEmpty()) {
firstMCS = new TreeMap<Integer, Integer>(allMCS.get(0));
firstMCS = new TreeMap<Integer, Integer>(allMCS.iterator().next());
}
}

private synchronized void setFirstAtomMapping() {
if (!allAtomMCS.isEmpty()) {
atomsMCS = new HashMap<IAtom, IAtom>(allAtomMCS.get(0));
atomsMCS = new HashMap<IAtom, IAtom>(allAtomMCS.iterator().next());
}

}

/** {@inheritDoc}
Expand Down
Expand Up @@ -138,10 +138,8 @@ private void postFilter() {
List<TreeMap<Integer, Integer>> SortedMap = new ArrayList<TreeMap<Integer, Integer>>();
connectedBondOrder = sortByValue(connectedBondOrder);
for (Map.Entry<Integer, Integer> map : connectedBondOrder.entrySet()) {

TreeMap<Integer, Integer> mapToBeMoved = mappings.get(map.getKey());
SortedMap.add(mapToBeMoved);

}
mappings = SortedMap;
FinalMappings final_MAPPINGS = FinalMappings.getInstance();
Expand Down
Expand Up @@ -31,7 +31,7 @@
import org.openscience.cdk.annotations.TestClass;
import org.openscience.cdk.annotations.TestMethod;
import org.openscience.cdk.smsd.helper.FinalMappings;
import org.openscience.cdk.smsd.helper.MolHandler;
import org.openscience.cdk.smsd.tools.MolHandler;
import org.openscience.cdk.smsd.interfaces.AbstractMCSAlgorithm;
import org.openscience.cdk.exception.CDKException;
import org.openscience.cdk.interfaces.IAtom;
Expand Down Expand Up @@ -193,11 +193,8 @@ private final synchronized void setAllAtomMapping() {

sourceAtom = source.getAtom(IIndex);
targetAtom = target.getAtom(JIndex);

atomMappings.put(sourceAtom, targetAtom);

}

allAtomMCS.add(counter++, atomMappings);
}
} catch (Exception I) {
Expand All @@ -208,14 +205,14 @@ private final synchronized void setAllAtomMapping() {
private synchronized void setFirstMapping() {

if (allMCS.size() > 0) {
firstMCS = new TreeMap<Integer, Integer>(allMCS.get(0));
firstMCS = new TreeMap<Integer, Integer>(allMCS.iterator().next());
}

}

private synchronized void setFirstAtomMapping() {
if (allAtomMCS.size() > 0) {
atomsMCS = new HashMap<IAtom, IAtom>(allAtomMCS.get(0));
atomsMCS = new HashMap<IAtom, IAtom>(allAtomMCS.iterator().next());
}

}
Expand Down
Expand Up @@ -36,7 +36,7 @@
import org.openscience.cdk.smsd.algorithm.vflib.interfaces.IQuery;
import org.openscience.cdk.smsd.algorithm.vflib.map.VFMapper;
import org.openscience.cdk.smsd.algorithm.vflib.query.TemplateCompiler;
import org.openscience.cdk.smsd.helper.MolHandler;
import org.openscience.cdk.smsd.tools.MolHandler;
import org.openscience.cdk.smsd.interfaces.AbstractSubGraph;
import org.openscience.cdk.interfaces.IAtom;
import org.openscience.cdk.interfaces.IAtomContainer;
Expand Down
Expand Up @@ -40,7 +40,7 @@
import org.openscience.cdk.smsd.algorithm.vflib.interfaces.IQuery;
import org.openscience.cdk.smsd.algorithm.vflib.map.VFMCSMapper;
import org.openscience.cdk.smsd.algorithm.vflib.query.TemplateCompiler;
import org.openscience.cdk.smsd.helper.MolHandler;
import org.openscience.cdk.smsd.tools.MolHandler;
import org.openscience.cdk.smsd.interfaces.AbstractMCSAlgorithm;
import org.openscience.cdk.interfaces.IAtom;
import org.openscience.cdk.interfaces.IAtomContainer;
Expand Down
Expand Up @@ -35,7 +35,7 @@
import org.openscience.cdk.smsd.algorithm.vflib.interfaces.INode;
import org.openscience.cdk.smsd.algorithm.vflib.interfaces.IQuery;
import org.openscience.cdk.smsd.algorithm.vflib.query.TemplateCompiler;
import org.openscience.cdk.smsd.helper.MolHandler;
import org.openscience.cdk.smsd.tools.MolHandler;
import org.openscience.cdk.smsd.interfaces.AbstractSubGraph;
import org.openscience.cdk.interfaces.IAtom;
import org.openscience.cdk.interfaces.IAtomContainer;
Expand Down
Expand Up @@ -26,14 +26,14 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import org.openscience.cdk.interfaces.IAtom;
import org.openscience.cdk.interfaces.IAtomContainer;
import org.openscience.cdk.smsd.algorithm.vflib.interfaces.IMapper;
import org.openscience.cdk.smsd.algorithm.vflib.interfaces.INode;
import org.openscience.cdk.smsd.algorithm.vflib.interfaces.IQuery;
import org.openscience.cdk.smsd.algorithm.vflib.interfaces.IState;
import org.openscience.cdk.smsd.algorithm.vflib.query.TemplateCompiler;
import org.openscience.cdk.smsd.algorithm.vflib.validator.VFMatch;
import org.openscience.cdk.interfaces.IAtom;
import org.openscience.cdk.interfaces.IAtomContainer;

/**
* This class finds MCS between query and target molecules
Expand Down
Expand Up @@ -33,7 +33,7 @@
import org.openscience.cdk.smsd.algorithm.mcsplus.MCSPlusHandler;
import org.openscience.cdk.smsd.algorithm.single.SingleMappingHandler;
import org.openscience.cdk.smsd.algorithm.vflib.VFlibMCSHandler;
import org.openscience.cdk.smsd.helper.MolHandler;
import org.openscience.cdk.smsd.tools.MolHandler;
import org.openscience.cdk.DefaultChemObjectBuilder;
import org.openscience.cdk.annotations.TestClass;
import org.openscience.cdk.annotations.TestMethod;
Expand Down
Expand Up @@ -48,7 +48,7 @@
import org.openscience.cdk.smsd.filters.ChemicalFilters;
import org.openscience.cdk.smsd.global.BondType;
import org.openscience.cdk.smsd.global.TimeOut;
import org.openscience.cdk.smsd.helper.MolHandler;
import org.openscience.cdk.smsd.tools.MolHandler;
import org.openscience.cdk.smsd.interfaces.Algorithm;
import org.openscience.cdk.smsd.interfaces.AbstractMCS;
import org.openscience.cdk.tools.ILoggingTool;
Expand Down Expand Up @@ -186,8 +186,6 @@ private synchronized void cdkMCSAlgorithm() {
firstAtomMCS.putAll(mcs.getFirstAtomMapping());
allAtomMCS.addAll(mcs.getAllAtomMapping());



}

private synchronized void mcsPlusAlgorithm() {
Expand Down Expand Up @@ -289,11 +287,7 @@ private boolean isBondMatch(IAtomContainer Reactant, IAtomContainer Product) {
}

private void defaultAlgorithm() {
if (BondType.getInstance().isBondSensitive()) {
cdkMCSAlgorithm();
} else {
mcsPlusAlgorithm();
}
cdkMCSAlgorithm();

This comment has been minimized.

Copy link
@egonw

egonw Jun 26, 2010

Member

This is an example of a big functional change. I should really do with:

  1. a good commit message
  2. a unit test, to test the new behavior

This comment has been minimized.

Copy link
@asad

asad Jun 26, 2010

Author Contributor

This is change after I have tested many molecules (12000 from KEGG). The MCSPlus is presently suffering from an indexing bug (was introduced at some point while optimising the code) which I will solve it soon (will refer my old svn log at EBI).

if (isTimeOut() || getFirstAtomMapping() == null) {
vfLibMCS();
}
Expand Down Expand Up @@ -514,20 +508,21 @@ public double getTanimotoSimilarity() throws IOException {
int decimalPlaces = 4;
int rAtomCount = 0;
int pAtomCount = 0;
double tanimoto = 0.0;
if (!removeHydrogen) {
rAtomCount = rMol.getMolecule().getAtomCount();
pAtomCount = pMol.getMolecule().getAtomCount();
} else {
rAtomCount = rMol.getMolecule().getAtomCount() - getHCount(rMol.getMolecule());
pAtomCount = pMol.getMolecule().getAtomCount() - getHCount(pMol.getMolecule());
}
double matchCount = getFirstMapping().size();
double tanimoto = (matchCount) / (rAtomCount + pAtomCount - matchCount);

BigDecimal tan = new BigDecimal(tanimoto);

tan = tan.setScale(decimalPlaces, BigDecimal.ROUND_HALF_UP);
tanimoto = tan.doubleValue();
if (getFirstMapping() != null || !getFirstMapping().isEmpty()) {
double matchCount = getFirstMapping().size();
tanimoto = (matchCount) / (rAtomCount + pAtomCount - matchCount);
BigDecimal tan = new BigDecimal(tanimoto);
tan = tan.setScale(decimalPlaces, BigDecimal.ROUND_HALF_UP);
tanimoto = tan.doubleValue();
}
return tanimoto;
}

Expand Down Expand Up @@ -610,26 +605,30 @@ public double getEuclideanDistance() throws IOException {
int decimalPlaces = 4;
double source = 0;
double target = 0;
double euclidean = -1;
if (!removeHydrogen) {
source = rMol.getMolecule().getAtomCount();
target = pMol.getMolecule().getAtomCount();
} else {
source = rMol.getMolecule().getAtomCount() - getHCount(rMol.getMolecule());
target = pMol.getMolecule().getAtomCount() - getHCount(pMol.getMolecule());
}
double common = getFirstMapping().size();
double euclidean = Math.sqrt(source + target - 2 * common);

BigDecimal dist = new BigDecimal(euclidean);
dist = dist.setScale(decimalPlaces, BigDecimal.ROUND_HALF_UP);
euclidean = dist.doubleValue();
if (getFirstMapping() != null || !getFirstMapping().isEmpty()) {
double common = getFirstMapping().size();
euclidean = Math.sqrt(source + target - 2 * common);
BigDecimal dist = new BigDecimal(euclidean);
dist = dist.setScale(decimalPlaces, BigDecimal.ROUND_HALF_UP);
euclidean = dist.doubleValue();
}
return euclidean;
}

/**
* {@inheritDoc}
* @return the bondSensitiveTimeOut
*/
@Override
public double getBondSensitiveTimeOut() {
return bondSensitiveTimeOut;
}
Expand All @@ -638,6 +637,7 @@ public double getBondSensitiveTimeOut() {
* {@inheritDoc}
* @param bondSensitiveTimeOut the bond Sensitive Timeout in mins (default 0.15 min)
*/
@Override
public void setBondSensitiveTimeOut(double bondSensitiveTimeOut) {
this.bondSensitiveTimeOut = bondSensitiveTimeOut;
}
Expand All @@ -646,6 +646,7 @@ public void setBondSensitiveTimeOut(double bondSensitiveTimeOut) {
* {@inheritDoc}
* @return the bondInSensitiveTimeOut
*/
@Override
public double getBondInSensitiveTimeOut() {
return bondInSensitiveTimeOut;
}
Expand All @@ -654,6 +655,7 @@ public double getBondInSensitiveTimeOut() {
* {@inheritDoc}
* @param bondInSensitiveTimeOut the bond insensitive Timeout in mins (default 0.15 min)
*/
@Override
public void setBondInSensitiveTimeOut(double bondInSensitiveTimeOut) {
this.bondInSensitiveTimeOut = bondInSensitiveTimeOut;
}
Expand Down

4 comments on commit 370a926

@egonw
Copy link
Member

@egonw egonw commented on 370a926 Jun 26, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asad, it is of utmost importance that you file bug report, that you cannot fix the same day. Or, if it has such an impact as the index bug seems to have, file it always. People need to know that they need to upgrade to the next version, if they use your SMSD code (and I know some are).

For these bugs, you must also add unit tests that shows at least one situation when the bug happens. And please annotate that unit test with "@cdk.bug XXXXX" then. It is really important to disclose these things.

@asad
Copy link
Contributor Author

@asad asad commented on 370a926 Jun 26, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your point is valid but I was waiting for the commits. I will file the bug report right away!
Thanks for -> ... "@cdk.bug XXXXX"

@asad
Copy link
Contributor Author

@asad asad commented on 370a926 Jun 26, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just filed the bug report. cdk.bug 3021679
Could you please add this to the MCSPlus module and then I will pull the changes.

@egonw
Copy link
Member

@egonw egonw commented on 370a926 Jun 26, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I do not have time for that now.

The @cdk.bug annotation goes on the test... check the existing source code; there is plenty of use of it.

Please sign in to comment.