Inheritance problem

[julienvienne]

Hello,

Inheritance seems to be a problem with the Earth class. Please have a look at the following example :

from pyearth import Earth

class EarthDerived(Earth):

    def __init__(self, other_attribute):
        Earth.__init__(self)
        self.other_attribute = other_attribute

if __name__ == "__main__":
    earth_derived = EarthDerived("foo")

/usr/bin/python3 /home/jv/pythonmars/trunk/pythonmars/examples/EarthInheritance.py
Traceback (most recent call last):
  File "/home/jv/pythonmars/trunk/pythonmars/examples/EarthInheritance.py", line 15, in <module>
    earth_derived = EarthDerived("foo")
  File "/home/jv/pythonmars/trunk/pythonmars/examples/EarthInheritance.py", line 11, in __init__
    Earth.__init__(self)
  File "/usr/local/lib/python3.4/dist-packages/py_earth-0.1.0-py3.4-linux-x86_64.egg/pyearth/earth.py", line 244, in __init__
    if call[name] is not None:
KeyError: 'other_attribute'

Regards

[mehdidc]

Hello @julienvienne,

thanks for reporting this issue. @jcrudy, this error happens because the class method _get_param_names (inherited from BaseEstimator) returns the param names of EarthDerived while it expects the param names of Earth. We might solve this issue by replacing self._get_param_names (in the init of Earth) by Earth._get_param_names, but some lines after self.set_params(...) is called and it expects the param names returned by self.get_params(...) which would be in this case the param names of EarthDerived, not those of Earth. I would suggest to use explicit assignments in the init of Earth, something like this :

      def __init__(self, max_terms=None, max_degree=None, allow_missing=False,
                   penalty=None, endspan_alpha=None, endspan=None,
                   minspan_alpha=None, minspan=None,
                   thresh=None, zero_tol=None, min_search_points=None,
                   check_every=None,
                   allow_linear=None, use_fast=None, fast_K=None,
                   fast_h=None, smooth=None, enable_pruning=True,
                   feature_importance=None, verbose=0):
          self.max_terms = max_terms
          self.max_degree = max_degree
          self.allow_missing = allow_missing
          self.penalty = penalty
          self.endspan_alpha = endspan_alpha
          self.endspan = endspan
          self.minspan_alpha = minspan_alpha
          self.minspan = minspan
          self.thresh = thresh
          self.zero_tol = zero_tol
          self.min_search_points = min_search_points
          self.check_every = check_every
          self.allow_linear = allow_linear
          self.use_fast = use_fast
          self.fast_K = fast_K
          self.fast_h = fast_h
          self.smooth = smooth
          self.enable_pruning = enable_pruning
          self.verbose = verbose

What do you think ? perhaps @agramfort might also have an opinion on this.

[agramfort]

+1 on @mehdidc suggestions. All params should be passed in init to follow sklearn convention. Another option is to write your own set_params and get_params methods.

[jcrudy]

It looks like the current Earth.__init__ carefully avoids setting attributes to None. I'm not sure, but I suspect it's code that I wrote and that I wrote it because I incorrectly thought that was the sklearn convention. So, since it's not the convention, I think it definitely makes sense to switch to @mehdidc's suggestion. There may be other places in the code that rely on an attribute being present or absent as some kind of indicator, although I can't think of any specifically.

[mehdidc]

"There may be other places in the code that rely on an attribute being present or absent as some kind of indicator, although I can't think of any specifically." yes exactly, actually in the constructor of ForwardPasser and PruningPasser.

[jcrudy]

Closing this as fixed by @mehdidc. @julienvienne please comment or reopen if you still experience the problem after the fix.