[Predator] Move ``SingleFeatureScore`` to LLM.

appengine/findit/crash/loglinear/training.py

Index: appengine/findit/crash/loglinear/training.py
diff --git a/appengine/findit/crash/loglinear/training.py b/appengine/findit/crash/loglinear/training.py
index f094848713c5f024c0cd5d594615c9c33e169f43..3538a51bf50eee3d0e2173c914478be09d45e3a6 100644
--- a/appengine/findit/crash/loglinear/training.py
+++ b/appengine/findit/crash/loglinear/training.py
@@ -33,7 +33,7 @@ class TrainableLogLinearModel(LogLinearModel):
         ``float``. N.B., the length of the list must be the same for all
         ``x`` and ``y``, and must be the same as the length of the list
         of weights.
-      initial_weights (list of float): the pre-training coefficients
+      initial_weights (dict from str to float): the pre-training coefficients
         for how much we believe components of the feature vector. This
         provides the seed for training; this starting value shouldn't
         affect the final weights obtained by training (thanks to
@@ -47,6 +47,7 @@ class TrainableLogLinearModel(LogLinearModel):
     super(TrainableLogLinearModel, self).__init__(
         Y_given_X, feature_function, initial_weights, epsilon)
     self._training_data = training_data
+    self._feature_order = initial_weights.keys()

[wrengr, 2017/01/11 20:38:30]

You should explicitly convert this to a list (or tuple) since those have a
specified order, rather than leaving it as a set which doesn't.

[Sharu Jiang, 2017/01/12 01:41:38]

I think the ``.keys()`` will return a list, but explicitly converting to a list
should be safer. Done.

[wrengr, 2017/01/12 18:16:16]

So it does. Weird.

 
     self._observed_feature_vector = vsum([
         self.FeaturesAsNumPyArray(x)(y)
@@ -65,33 +66,26 @@ class TrainableLogLinearModel(LogLinearModel):
     return self._weights
 
   @weights.setter
-  def weights(self, new_weights): # pylint: disable=W0221
+  def weights(self, new_np_weights): # pylint: disable=W0221
     """Mutate the weight covector, and clear memos as necessary.
 
     This setter attempts to avoid clearing memos whenever possible,
     but errs on the side of caution/correctness when it needs to.
 
     Args:
-      new_weights (np.ndarray): the new weights to use. Must have the
-        same shape as the old ``np.ndarray``.
+      new_np_weights (np.ndarray): the new weights to use. It will be converted
+      to weights dict mapping feature_name to its weight.
     """
-    if new_weights is self._weights:
-      return
-
-    if not isinstance(new_weights, np.ndarray):
-      raise TypeError('Expected an np.ndarray but got %s instead'
-          % new_weights.__class__.__name__)
-
-    if new_weights.shape != self._weights.shape:
-      raise TypeError('Weight shape mismatch: %s != %s'
-          % (new_weights.shape, self._weights.shape))
-
+    new_weights = self.NumPyArrayToWeights(new_np_weights)
     self.ClearWeightBasedMemos()
     self._weights = new_weights

[wrengr, 2017/01/11 20:38:31]

To avoid excessive conversion back and forth, this class should store both the
dict and np.ndarray. That is, I suggest:

    def __init__(self,...):
        ...
        self._weights = initial_weights
        self._np_weights = self._DictToNpArray(initial_weights, 0.)

    @property
    def weights(self):
        return self._weights

    @weights.setter
    def weights(self, weights_dict):
        self.ClearWeightBasedMemos()
        self._weights = weights_dict
        self._np_weights = self._DictToNpArray(weights_dict, 0.)

    @property
    def np_weights(self):
        return self._np_weights

    @np_weights.setter
    def np_weights(self, weights_array):
        self.ClearWeightBasedMemos()
        self._weights = self._NpArrayToDict(weights_array, self._LogZeroish)
        self._np_weights = weights_array

    def _DictToNpArray(self, d, default=None):
        return np.array([d.get(i, default) for i in self._feature_order])

    def _NpArrayToDict(self, arr, predicate=None):
        if predicate is None:
          predicate = lambda _: True
        return {k: v for k, v in zip(self._feature_order, arr) if predicate(v)}

[Sharu Jiang, 2017/01/12 01:41:38]

Done.

 
   def FeaturesAsNumPyArray(self, x):
     """A variant of ``Features`` which returns a ``np.ndarray``.
 
+    Note, the features nparray should have the same order as in
+    self._feature_order to stay aligned with weights np array.
+
     For training we need to have the feature function return an
     ``np.ndarray(float)`` rather than the ``list(FeatureValue)`` used
     elsewhere. This function performes the necessary conversion.
@@ -103,7 +97,44 @@ class TrainableLogLinearModel(LogLinearModel):
     bottleneck, we can add the extra layer of memoization to avoid that.
     """
     fx = self.Features(x)
-    return lambda y: np.array([fxy.value for fxy in fx(y)])
+    def FeaturesAsNumPyArrayGivenX(y):
+      fxys = fx(y)
+      return np.array([fxys[feature_name].value if feature_name in fxys else 0.

[wrengr, 2017/01/11 20:38:30]

is clearer to use ``fxys.get(feature_name, 0.)``

[Sharu Jiang, 2017/01/12 01:41:38]

I have to do this, because I need ``fxys[feature_name].value`` instead of
``fxys[feature_name]``

[wrengr, 2017/01/12 18:16:16]

hrm... that does make it tricky.

This sort of thing is why I had FeatureValue.__float__ previously; with that we'
could've ``float(fxys.get(feature_name, 0.))`` without needing to wrap ``0.`` up
in something that provides a ``.value`` property. I really wish Python had
let-bindings in its comprehension syntax. One alternative is to build an
intermediate generator, thus: ``return np.array([0. if fxyn is None else
fxyn.value for fxyn in (fxys.get(n) for n in self._feature_order)])``. Not sure
that that's any cleaner though :(

+                       for feature_name in self._feature_order])
+
+    return FeaturesAsNumPyArrayGivenX
+
+  def WeightsAsNumPyArray(self):
+    """Converts dict (mapping feature name to weight) to numpy array.
+
+    Note, this conversion is needed because model uses weights dict to organize
+    weights for features, however SciPy trainning (e.g. BFGS) needs numpy array
+    to do computaion.
+    """
+    return np.array([self._weights.get(feature_name, 0.)
+                     for feature_name in self._feature_order])

[wrengr, 2017/01/11 20:38:31]

Again, to avoid excessive conversion, we can do this once in the ``weights``
setter and then this method simply returns the cached np.array

[Sharu Jiang, 2017/01/12 01:41:38]

Done.

+
+  def NumPyArrayToWeights(self, np_weights):
+    """Converts numpy array to dict (mapping feature name to weight).
+
+    Note, this conversion is needed because model uses weights dict to organize
+    weights for features, however SciPy trainning (e.g. BFGS) needs numpy array
+    to do computaion.
+
+    Args:
+      np_weights (np.ndarray): Weights which have the same order of
+        self._feature_order. Note, featuer np array should also be serialized by
+        the same order as self._feature_order to match.
+
+    Returns:
+      A dict mapping feature name to weight.
+    """
+    if not isinstance(np_weights, np.ndarray):
+      raise TypeError('Expected an np.ndarray but got %s instead'
+          % np_weights.__class__.__name__)
+
+    return {feature_name: weight
+            for feature_name, weight in zip(self._feature_order, np_weights)}
 
   def LogLikelihood(self):
     """The conditional log-likelihood of the training data.
@@ -122,7 +153,8 @@ class TrainableLogLinearModel(LogLinearModel):
     will be the log-likelihood plus some penalty terms for regularization.
     """
     observed_zeta = math.fsum(self.LogZ(x) for x, _ in self._training_data)
-    observed_score = self.weights.dot(self._observed_feature_vector)
+    observed_score = self.WeightsAsNumPyArray().dot(
+        self._observed_feature_vector)
     return observed_score - observed_zeta
 
   def LogLikelihoodGradient(self):
@@ -142,7 +174,7 @@ class TrainableLogLinearModel(LogLinearModel):
     Returns:
       Nothing, but has the side effect of mutating the stored weights.
     """
-    initial_weights = self.weights
+    initial_weights = self.WeightsAsNumPyArray()
 
     # We want to minimize the number of times we reset the weights since
     # that clears our memos. One might think we could do that in the
@@ -158,7 +190,7 @@ class TrainableLogLinearModel(LogLinearModel):
 
     def objective_function_gradient(new_weights):
       self.weights = new_weights
-      return -self.LogLikelihoodGradient() + l2_penalty * self.weights
+      return -self.LogLikelihoodGradient() + l2_penalty * new_weights

[wrengr, 2017/01/11 20:38:31]

I don't think that's any cleaner than the old code

[Sharu Jiang, 2017/01/12 01:41:38]

This is because we need np_array here instead of a dict.

[wrengr, 2017/01/12 18:16:16]

Ah, right <chagrin>

 
     result = spo.minimize(
         objective_function,