Clean-up Python #4815 - P1.5: Chem\FeatMaps (#4894)

* Clean-up Python #4815 - P1.5: Chem\FeatMaps

This is the sub-PR breaking from the large PR 4815: https://github.com/rdkit/rdkit/pull/4815

--------------------
Updated Changes
- [New]: Cleanup `class FeatMapParser`,
- [Recall]: Function `ScoreFeats()` changed the default variables for data/memory safety. Check in PR 4815. This may cause backward incompatible
- [Recall]: Function `GetFeatFeatDistMatrix()` is optimized.

* Adjust codebase by review/comment

* Update FeatMapParser.py

* Update FeatMapParser.py

* Update FeatMaps.py

rdkit/Chem/FeatMaps/FeatMapParser.py

@@ -146,9 +146,7 @@ class FeatMapParser(object):
     splitL = txt.split(',')
     if len(splitL) != 3:
       raise ValueError('Bad location string')
-    vs = [float(x) for x in splitL]
-    pt = Geometry.Point3D(vs[0], vs[1], vs[2])
-    return pt
+    return Geometry.Point3D(float(splitL[0]), float(splitL[1]), float(splitL[2]))
 
   def ParseFeatPointBlock(self):
     featLineSplitter = re.compile(r'([a-zA-Z]+) *= *')
@@ -160,32 +158,20 @@ class FeatMapParser(object):
       while vals.count(''):
         vals.remove('')
       p = FeatMapPoint.FeatMapPoint()
-
-      i = 0
-      while i < len(vals):
+      for i in range(0, len(vals), 2):
         name = vals[i].lower()
+        value = vals[i + 1]
         if name == 'family':
-          i += 1
-          val = vals[i].strip()
-          p.SetFamily(val)
+          p.SetFamily(value.strip())
         elif name == 'weight':
-          i += 1
-          val = float(vals[i])
-          p.weight = val
+          p.weight = float(value)
         elif name == 'pos':
-          i += 1
-          val = vals[i]
-          pos = self._parsePoint(val)
-          p.SetPos(pos)
+          p.SetPos(self._parsePoint(value))
         elif name == 'dir':
-          i += 1
-          val = vals[i]
-          pos = self._parsePoint(val)
-          p.featDirs.append(pos)
+          p.featDirs.append(self._parsePoint(value))
         else:
-          raise FeatMapParseError('FeatPoint option %s not recognized on line %d' %
-                                  (name, self._lineNum))
-        i += 1
+          raise FeatMapParseError(f'FeatPoint option {name} not recognized on line {self._lineNum}')
+        
       feats.append(p)
       l = self._NextLine()
     return feats

rdkit/Chem/FeatMaps/FeatMapUtils.py

@@ -71,32 +71,32 @@ def GetFeatFeatDistMatrix(fm, mergeMetric, mergeTol, dirMergeMode, compatFunc):
 
   """
   MergeMetric.valid(mergeMetric)
-
-  dists = [[1e8] * fm.GetNumFeatures() for _ in range(fm.GetNumFeatures())]
+  numFeatures = fm.GetNumFeatures()
+  dists = [[1e8] * numFeatures for _ in range(numFeatures)]
   if mergeMetric == MergeMetric.NoMerge:
     return dists
-  elif mergeMetric == MergeMetric.Distance:
-    mergeTol2 = mergeTol * mergeTol
-    for i in range(fm.GetNumFeatures()):
-      ptI = fm.GetFeature(i)
-      for j in range(i + 1, fm.GetNumFeatures()):
-        ptJ = fm.GetFeature(j)
-        if compatFunc(ptI, ptJ):
+  
+  # Setup distance matrix, depending on mergeMetric. 
+  benchmarkDict = { MergeMetric.Distance: mergeTol * mergeTol, MergeMetric.Overlap: mergeTol}
+  benchmark = benchmarkDict[mergeMetric]
+  def assignMatrix(matrix, i, j, value, constraint):
+    if value < constraint:
+      matrix[i][j] = value
+      matrix[j][i] = value
+
+  getFeature = fm.GetFeature
+  for i in range(numFeatures):
+    ptI = getFeature(i)
+    for j in range(i + 1, numFeatures):
+      ptJ = getFeature(j)
+      if compatFunc(ptI, ptJ):
+        if mergeMetric == MergeMetric.Distance:
           dist2 = ptI.GetDist2(ptJ)
-          if dist2 < mergeTol2:
-            dists[i][j] = dist2
-            dists[j][i] = dist2
-  elif mergeMetric == MergeMetric.Overlap:
-    for i in range(fm.GetNumFeatures()):
-      ptI = fm.GetFeature(i)
-      for j in range(i + 1, fm.GetNumFeatures()):
-        ptJ = fm.GetFeature(j)
-        if compatFunc(ptI, ptJ):
-          score = fm.GetFeatFeatScore(ptI, ptJ, typeMatch=False)
-          score *= -1 * ptJ.weight
-          if score < mergeTol:
-            dists[i][j] = score
-            dists[j][i] = score
+          assignMatrix(matrix=dists, i=i, j=j, value=dist2, constraint=benchmark)
+        elif mergeMetric == MergeMetric.Overlap:
+          score = fm.GetFeatFeatScore(ptI, ptJ, typeMatch=False) * (-1 * ptJ.weight)
+          assignMatrix(matrix=dists, i=i, j=j, value=score, constraint=benchmark)
+
   return dists
 
 
@@ -128,8 +128,7 @@ def MergeFeatPoints(fm, mergeMetric=MergeMetric.NoMerge, mergeTol=1.5,
     return res
   dists = GetFeatFeatDistMatrix(fm, mergeMetric, mergeTol, dirMergeMode, compatFunc)
   distOrders = [None] * len(dists)
-  for i in range(len(dists)):
-    distV = dists[i]
+  for i, distV in enumerate(dists):
     distOrders[i] = []
     for j, dist in enumerate(distV):
       if dist < mergeTol:
@@ -166,7 +165,7 @@ def MergeFeatPoints(fm, mergeMetric=MergeMetric.NoMerge, mergeTol=1.5,
       else:
         # it may be that there are several points at about the same distance,
         # check for that now
-        if (feq(distOrders[nbr][0][0], dist)):
+        if feq(distOrders[nbr][0][0], dist):
           for distJ, nbrJ in distOrders[nbr][1:]:
             if feq(dist, distJ):
               if nbrJ == fi:
@@ -178,6 +177,7 @@ def MergeFeatPoints(fm, mergeMetric=MergeMetric.NoMerge, mergeTol=1.5,
       # print '    bottom:',mergeThem
       if mergeThem:
         break
+    
     if mergeThem:
       res = True
       featI = fm.GetFeature(fi)

rdkit/Chem/FeatMaps/FeatMaps.py

@@ -122,7 +122,6 @@ class FeatMap(object):
     """ feat1 is one of our feats
         feat2 is any Feature
 
-
     """
     if typeMatch and feat1.GetFamily() != feat2.GetFamily():
       return 0.0
@@ -141,8 +140,7 @@ class FeatMap(object):
         score = 0.0
     elif params.featProfile == FeatMapParams.FeatProfile.Box:
       score = 1.0
-    weight = feat1.weight
-    score *= weight
+    score *= feat1.weight
 
     if self.dirScoreMode != FeatDirScoreMode.Ignore:
       dirScore = feat1.GetDirMatch(feat2)
@@ -154,41 +152,42 @@ class FeatMap(object):
 
     return score
 
-  def ScoreFeats(self, featsToScore, mapScoreVect=[], featsScoreVect=[], featsToFeatMapIdx=[]):
+  def ScoreFeats(self, featsToScore, mapScoreVect=None, featsScoreVect=None, featsToFeatMapIdx=None):
     nFeats = len(self._feats)
-    if mapScoreVect and len(mapScoreVect) != nFeats:
-      raise ValueError('if provided, len(mapScoreVect) should equal numFeats')
-    nToScore = len(featsToScore)
-    if featsScoreVect and len(featsScoreVect) != nToScore:
-      raise ValueError('if provided, len(featsScoreVect) should equal len(featsToScore)')
-    if featsToFeatMapIdx and len(featsToFeatMapIdx) != nToScore:
-      raise ValueError('if provided, len(featsToFeatMapIdx) should equal len(featsToScore)')
-
-    if mapScoreVect:
+    if mapScoreVect is not None:
+      if len(mapScoreVect) != nFeats:
+        raise ValueError('if provided, len(mapScoreVect) should equal numFeats')
       for i in range(nFeats):
         mapScoreVect[i] = 0.0
     else:
       mapScoreVect = [0.0] * nFeats
 
+    nToScore = len(featsToScore)
     if self.scoreMode == FeatMapScoreMode.Closest:
       defScore = 1000.0
-    else:
+    else: 
       defScore = 0.0
-    if featsScoreVect:
+      
+    if featsScoreVect is not None:
+      if len(featsScoreVect) != nToScore:
+        raise ValueError('if provided, len(featsScoreVect) should equal len(featsToScore)')
       for i in range(nToScore):
         featsScoreVect[i] = defScore
     else:
       featsScoreVect = [defScore] * nToScore
 
-    if not featsToFeatMapIdx:
+    if featsToFeatMapIdx is not None: # Initialize a 2D-empty array
+      if len(featsToFeatMapIdx) != nToScore:
+        raise ValueError('if provided, len(featsToFeatMapIdx) should equal len(featsToScore)')
+    else:
       featsToFeatMapIdx = [None] * nToScore
-
+    
     for i in range(nToScore):
       if self.scoreMode != FeatMapScoreMode.All:
         featsToFeatMapIdx[i] = [-1]
       else:
         featsToFeatMapIdx[i] = []
-
+    
     for oIdx, oFeat in enumerate(featsToScore):
       for sIdx, sFeat in self._loopOverMatchingFeats(oFeat):
         if self.scoreMode == FeatMapScoreMode.Closest:
@@ -220,7 +219,6 @@ class FeatMap(object):
           totScore += lScore
         else:
           featsScoreVect[oIdx] = 0
-
     else:
       totScore = sum(featsScoreVect)
       if self.scoreMode == FeatMapScoreMode.Best: