biolab/orange3 — pull request #188 by janezd (2014-12-27 21:44:42+00:00)
Title: Refactor and document Orange.data.Variable

Beside some refactoring and documenting, I also
- got rid of get_value_locks,
- which simplified pickling - I could remove the methods for saving and retrieving state,
- moved "ordered" to DiscreteVariables,
- fixed bugs in adjustment of number of decimals (and added the corresponding unit tests).

@astaric, get_value_from used to be called directly until I discovered (some time ten years ago) that one can create a cycle of get_value_froms, so I added compute_value and locking. We can safely assume that no sane person would create such cycles, which is why I now removed these ancient locks. I guess this eliminates the need for get_value_from. Variable.compute_value would always return data.value.Unknown. For variables that compute their values from source variables, we would replace the instance's compute_value with the instance-specific function, e.g. var.compute_value = my_discretizater. The only caveat is that my_discretizer is a function, not a method. There are two solutions. First is to have something like:

@property
def compute_value(self):
return self._compute_value

@compute_value.setter
def compute_value(self, f):
self._compute_value = types.MethodType(f, self)

def _compute_value(self, _):
return Unknown

I tried to run this through unit tests and failed (I didn't push hard, though).

A simpler solution is to make compute_value a static method.

@staticmethod
def compute_value(self, _):
return Unknown

If we'd ever to access the variable (we don't have it with the current get_value_from, and I can't image a scenario when we'd need that, at least not in the current implementation of discretization), we can put the "self" into the closure.

The benefit of the first approach is simpler access to self (why?), the benefit of the latter is simplicity. After just replacing the occurrences of get_value_from with compute_value, the latter survives the unit tests (except, for now, for those that directly test the type of get_value_from).

Despite this subtleties, I think we should prefer having a single compute_value instead of the mysterious pair compute_value / get_value_from.