Skip to content

Commit 2603f33

Browse files
asadasad
committed
Chemical robustness: fix stereo logic, deterministic maps, aromatic bonds
Chemical correctness: - BondChangeAnnotator: Fix E/Z stereo detection logic error — was (E OR Z) OR (Z OR E) which is always true; now correctly detects E⇌Z and R⇌S inversions only when stereo actually changes - BondChange: Handle aromatic bonds (isAromatic flag) and null bond orders instead of silently defaulting to single bond - StandardizeReaction: Add atom balance validation with warning log for unbalanced reactions (graceful, does not fail) Robustness: - BondChangeCalculator: Null-safe property access using CONSTANT.equals(obj.getProperty()) pattern (3 locations) - BondChangeCalculator: Replace all HashMap/HashSet with LinkedHashMap/LinkedHashSet for deterministic iteration order — ensures reproducible fingerprints across JVM runs - BondChange: Remove unnecessary synchronized from pure getters All 40 tests pass. Author: Syed Asad Rahman <asad.rahman@bioinceptionlabs.com>
1 parent 17b44ad commit 2603f33

4 files changed

Lines changed: 94 additions & 64 deletions

File tree

‎src/main/java/com/bioinceptionlabs/reactionblast/mechanism/BondChangeAnnotator.java‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -312,16 +312,14 @@ protected synchronized void markBondChanges() throws Exception {
312312
getReactionCenterSet().add(atomE);
313313
getReactionCenterSet().add(atomP);
314314

315-
if ((sc.getReactantAtomStereo().equals(E)
316-
|| sc.getProductAtomStereo().equals(Z))
317-
|| (sc.getReactantAtomStereo().equals(Z)
318-
|| sc.getProductAtomStereo().equals(E))) {
319-
getConformationChangeList().add(new AtomStereoChangeInformation(atomE, atomP, sc.getReactantAtomStereo(), sc.getProductAtomStereo()));
320-
} else if ((sc.getReactantAtomStereo().equals(R)
321-
|| sc.getProductAtomStereo().equals(S))
322-
|| (sc.getReactantAtomStereo().equals(S)
323-
|| sc.getProductAtomStereo().equals(R))) {
324-
getStereoChangeList().add(new AtomStereoChangeInformation(atomE, atomP, sc.getReactantAtomStereo(), sc.getProductAtomStereo()));
315+
if ((rsb.equals(E) && psb.equals(Z))
316+
|| (rsb.equals(Z) && psb.equals(E))) {
317+
// E⇌Z geometric isomerization detected
318+
getConformationChangeList().add(new AtomStereoChangeInformation(atomE, atomP, rsb, psb));
319+
} else if ((rsb.equals(R) && psb.equals(S))
320+
|| (rsb.equals(S) && psb.equals(R))) {
321+
// R⇌S stereocenter inversion detected
322+
getStereoChangeList().add(new AtomStereoChangeInformation(atomE, atomP, rsb, psb));
325323
}
326324
}
327325
}

‎src/main/java/com/bioinceptionlabs/reactionblast/mechanism/BondChangeCalculator.java‎

Lines changed: 20 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,7 @@
2626
import java.util.ArrayList;
2727
import java.util.Collection;
2828
import static java.util.Collections.unmodifiableCollection;
29-
import java.util.HashMap;
30-
import java.util.HashSet;
29+
import java.util.LinkedHashMap;
3130
import java.util.LinkedHashSet;
3231
import java.util.List;
3332
import java.util.Map;
@@ -140,18 +139,18 @@ public BondChangeCalculator(IReaction reaction) throws Exception {
140139
this.reactionCenterWFingerprint = new PatternFingerprinter();
141140
this.reactionCenterWFingerprint.setFingerprintID(reaction.getID() + ":" + "Reaction Center");
142141

143-
this.reactionCenterFormedCleavedFingerprint = new HashMap<>();
144-
this.reactionCenterOrderChangeFingerprint = new HashMap<>();
145-
this.reactionCenterStereoChangeFingerprint = new HashMap<>();
142+
this.reactionCenterFormedCleavedFingerprint = new LinkedHashMap<>();
143+
this.reactionCenterOrderChangeFingerprint = new LinkedHashMap<>();
144+
this.reactionCenterStereoChangeFingerprint = new LinkedHashMap<>();
146145

147146
this.reactionMoleculeMoleculePairList = new LinkedHashSet<>();
148147

149-
this.bondFormedMap = new HashMap<>();
150-
this.bondCleavedMap = new HashMap<>();
151-
this.bondOrderRMap = new HashMap<>();
152-
this.bondOrderPMap = new HashMap<>();
153-
this.AtomStereoRMap = new HashMap<>();
154-
this.AtomStereoPMap = new HashMap<>();
148+
this.bondFormedMap = new LinkedHashMap<>();
149+
this.bondCleavedMap = new LinkedHashMap<>();
150+
this.bondOrderRMap = new LinkedHashMap<>();
151+
this.bondOrderPMap = new LinkedHashMap<>();
152+
this.AtomStereoRMap = new LinkedHashMap<>();
153+
this.AtomStereoPMap = new LinkedHashMap<>();
155154
this.reactionCenterFragmentList = new ArrayList<>();
156155
}
157156

@@ -714,7 +713,7 @@ public IReaction getReactionWithCompressUnChangedHydrogens() {
714713
compressedReaction = deepClone(mappedReaction);
715714
compressedReaction.setProperties(mappedReaction.getProperties());
716715
//Add mapping to the clone
717-
Map<IAtom, IAtom> mappings = new HashMap<>();
716+
Map<IAtom, IAtom> mappings = new LinkedHashMap<>();
718717
for (IMapping mapping : compressedReaction.mappings()) {
719718
mappings.put((IAtom) mapping.getChemObject(0), (IAtom) mapping.getChemObject(1));
720719
}
@@ -1169,8 +1168,8 @@ public void computeBondChanges(boolean generate2D, boolean generate3D) throws CD
11691168
/*
11701169
* Loop over atom order and generate unique list to atoms
11711170
*/
1172-
Set<IAtom> reactantAtoms = new HashSet<>();
1173-
Set<IAtom> productAtoms = new HashSet<>();
1171+
Set<IAtom> reactantAtoms = new LinkedHashSet<>();
1172+
Set<IAtom> productAtoms = new LinkedHashSet<>();
11741173

11751174
LOGGER.debug("Bond Change List: " + bondChangeAnnotator.getBondChangeList().size());
11761175

@@ -1180,10 +1179,8 @@ public void computeBondChanges(boolean generate2D, boolean generate3D) throws CD
11801179

11811180
// Mark Bond Order Changes
11821181
if (bondR != null && bondP != null
1183-
&& bondP.getProperties().get(BOND_CHANGE_INFORMATION).
1184-
equals(BOND_ORDER)
1185-
&& bondR.getProperties().get(BOND_CHANGE_INFORMATION).
1186-
equals(BOND_ORDER)) {
1182+
&& BOND_ORDER.equals(bondP.getProperties().get(BOND_CHANGE_INFORMATION))
1183+
&& BOND_ORDER.equals(bondR.getProperties().get(BOND_CHANGE_INFORMATION))) {
11871184

11881185
bondOrderRMap.put(bondR, getMoleculeID(bondR, mappedReaction.getReactants()));
11891186
bondR.getAtom(0).setProperty(BOND_CHANGE_INFORMATION, BOND_ORDER);
@@ -1235,10 +1232,8 @@ public void computeBondChanges(boolean generate2D, boolean generate3D) throws CD
12351232
LOGGER.debug("Bond formed, cleaved changes 1 ");
12361233

12371234
//Mark Formed Bonds in the Product
1238-
if (bondP != null && (bondP.getProperties().get(BOND_CHANGE_INFORMATION).
1239-
equals(BOND_FORMED)
1240-
|| bondP.getProperties().get(BOND_CHANGE_INFORMATION).
1241-
equals(PSEUDO_BOND))) {
1235+
if (bondP != null && (BOND_FORMED.equals(bondP.getProperties().get(BOND_CHANGE_INFORMATION))
1236+
|| PSEUDO_BOND.equals(bondP.getProperties().get(BOND_CHANGE_INFORMATION)))) {
12421237

12431238
LOGGER.debug("Bond formed, cleaved changes 1 - 1");
12441239

@@ -1291,10 +1286,8 @@ public void computeBondChanges(boolean generate2D, boolean generate3D) throws CD
12911286
LOGGER.debug("Bond formed, cleaved changes 2");
12921287

12931288
//Mark Cleaved Bonds in Reactants
1294-
if (bondR != null && (bondR.getProperties().get(BOND_CHANGE_INFORMATION).
1295-
equals(BOND_CLEAVED)
1296-
|| bondR.getProperties().get(BOND_CHANGE_INFORMATION).
1297-
equals(PSEUDO_BOND))) {
1289+
if (bondR != null && (BOND_CLEAVED.equals(bondR.getProperties().get(BOND_CHANGE_INFORMATION))
1290+
|| PSEUDO_BOND.equals(bondR.getProperties().get(BOND_CHANGE_INFORMATION)))) {
12981291

12991292
LOGGER.debug("Bond formed, cleaved changes 1 - 2 - 1");
13001293

@@ -1342,7 +1335,7 @@ public void computeBondChanges(boolean generate2D, boolean generate3D) throws CD
13421335
/*
13431336
* IMP for RC Fingerprint: compute all the unique mappedReaction centers atoms
13441337
*/
1345-
Map<IAtom, IAtom> reactionCenterMap = new HashMap<>();
1338+
Map<IAtom, IAtom> reactionCenterMap = new LinkedHashMap<>();
13461339
bondChangeAnnotator.getReactionCenterSet().stream().filter((atom) -> (!atom.getSymbol().equals("H"))).forEachOrdered((atom) -> {
13471340
reactionCenterMap.put(atom, bondChangeAnnotator.getMappingMap().get(atom));
13481341
});

‎src/main/java/com/bioinceptionlabs/reactionblast/mechanism/helper/BondChange.java‎

Lines changed: 16 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -38,33 +38,31 @@ public class BondChange implements Serializable {
3838
* @param bond
3939
* @return
4040
*/
41-
public static synchronized int convertBondOrder(IBond bond) {
42-
int value;
41+
public static int convertBondOrder(IBond bond) {
42+
if (bond.getOrder() == null) {
43+
return bond.isAromatic() ? 2 : 1;
44+
}
4345
switch (bond.getOrder()) {
4446
case QUADRUPLE:
45-
value = 4;
46-
break;
47+
return 4;
4748
case TRIPLE:
48-
value = 3;
49-
break;
49+
return 3;
5050
case DOUBLE:
51-
value = 2;
52-
break;
51+
return 2;
5352
case SINGLE:
54-
value = 1;
55-
break;
53+
return 1;
5654
default:
57-
value = 1;
55+
// Handle UNSET or other cases — check aromaticity flag
56+
return bond.isAromatic() ? 2 : 1;
5857
}
59-
return value;
6058
}
6159

6260
/**
6361
*
6462
* @param bond
6563
* @return
6664
*/
67-
public static synchronized int convertBondStereo(IBond bond) {
65+
public static int convertBondStereo(IBond bond) {
6866
int value;
6967
switch (bond.getStereo()) {
7068
case UP:
@@ -124,33 +122,33 @@ public BondChange(IBond reactantBond, IBond productBond) {
124122
/**
125123
* @return the reactantBond
126124
*/
127-
public synchronized IBond getReactantBond() {
125+
public IBond getReactantBond() {
128126
return reactantBond;
129127
}
130128

131129
/**
132130
* @return the productBond
133131
*/
134-
public synchronized IBond getProductBond() {
132+
public IBond getProductBond() {
135133
return productBond;
136134
}
137135

138136
/**
139137
* @return the bondChangeDelta
140138
*/
141-
public synchronized float getBondChangeDelta() {
139+
public float getBondChangeDelta() {
142140
return bondChangeDelta;
143141
}
144142

145143
/**
146144
* @param bondChangeDelta the bondChangeDelta to set
147145
*/
148-
private synchronized void setBondChangeInformation(float bondChangeInformation) {
146+
private void setBondChangeInformation(float bondChangeInformation) {
149147
this.bondChangeDelta = bondChangeInformation;
150148
}
151149

152150
@Override
153-
public synchronized String toString() {
151+
public String toString() {
154152
StringBuilder result = new StringBuilder();
155153
result.append("\t");
156154
result.append(NEW_LINE);

‎src/main/java/com/bioinceptionlabs/reactionblast/tools/StandardizeReaction.java‎

Lines changed: 50 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -19,36 +19,77 @@
1919
package com.bioinceptionlabs.reactionblast.tools;
2020

2121
import static java.lang.System.currentTimeMillis;
22+
import java.util.LinkedHashMap;
23+
import java.util.Map;
24+
import java.util.logging.Level;
2225

26+
import org.openscience.cdk.interfaces.IAtom;
27+
import org.openscience.cdk.interfaces.IAtomContainer;
2328
import org.openscience.cdk.interfaces.IReaction;
29+
import org.openscience.cdk.tools.ILoggingTool;
30+
import static org.openscience.cdk.tools.LoggingToolFactory.createLoggingTool;
2431
import com.bioinceptionlabs.reactionblast.interfaces.IStandardizer;
2532
import com.bioinceptionlabs.reactionblast.mapping.container.CDKReactionBuilder;
2633
import static com.bioinceptionlabs.reactionblast.mapping.helper.MappingHandler.cleanMapping;
2734

2835
/**
36+
* Standardizes reaction objects for atom-atom mapping.
37+
* Validates atom balance and prepares reaction containers.
2938
*
3039
* @contact Syed Asad Rahman, BioInception.
3140
* @author Syed Asad Rahman <asad.rahman@bioinceptionlabs.com>
3241
*/
3342
public class StandardizeReaction implements IStandardizer {
3443

44+
private static final ILoggingTool LOGGER = createLoggingTool(StandardizeReaction.class);
45+
3546
/**
47+
* Standardize a reaction: clean mappings, validate balance, build containers.
3648
*
37-
* @param reaction
49+
* @param reaction the input reaction
3850
* @return New Standardized reaction Object
39-
* @throws Exception
51+
* @throws Exception if standardization fails
4052
*/
4153
@Override
42-
public synchronized IReaction standardize(IReaction reaction) throws Exception {
43-
String ReactionID = reaction.getID();
54+
public IReaction standardize(IReaction reaction) throws Exception {
55+
String reactionID = reaction.getID();
4456
cleanMapping(reaction);
4557

46-
if (ReactionID == null) {
47-
ReactionID = Long.toString(currentTimeMillis());
48-
reaction.setID(ReactionID);
58+
if (reactionID == null) {
59+
reactionID = Long.toString(currentTimeMillis());
60+
reaction.setID(reactionID);
4961
}
62+
63+
// Validate atom balance (warn but don't fail — some reactions are intentionally unbalanced)
64+
checkAtomBalance(reaction);
65+
5066
CDKReactionBuilder rBuilder = new CDKReactionBuilder();
51-
IReaction standardizedReaction = rBuilder.standardize(reaction);
52-
return standardizedReaction;
67+
return rBuilder.standardize(reaction);
68+
}
69+
70+
/**
71+
* Check if a reaction is atom-balanced. Logs a warning if not.
72+
* Does not throw — unbalanced reactions are handled gracefully.
73+
*
74+
* @param reaction the reaction to check
75+
*/
76+
private void checkAtomBalance(IReaction reaction) {
77+
Map<String, Integer> reactantAtoms = countAtoms(reaction.getReactants());
78+
Map<String, Integer> productAtoms = countAtoms(reaction.getProducts());
79+
80+
if (!reactantAtoms.equals(productAtoms)) {
81+
LOGGER.warn("Reaction " + reaction.getID() + " may be unbalanced: "
82+
+ "reactants=" + reactantAtoms + " products=" + productAtoms);
83+
}
84+
}
85+
86+
private Map<String, Integer> countAtoms(org.openscience.cdk.interfaces.IAtomContainerSet molSet) {
87+
Map<String, Integer> counts = new LinkedHashMap<>();
88+
for (IAtomContainer mol : molSet.atomContainers()) {
89+
for (IAtom atom : mol.atoms()) {
90+
counts.merge(atom.getSymbol(), 1, Integer::sum);
91+
}
92+
}
93+
return counts;
5394
}
5495
}

0 commit comments

Comments
 (0)