Cross Validation Using New Decorator API

[rmorshea]

brief example

from traitlets import HasTraits, Int, validate

class H(HasTraits):

    i = Int()

    @validate('i')
    def _i_validator(self, proposal):
        return proposal['value']+1

h  = H()
h.i = 1
print('result : '+ str(h.i))

'result : 2'

[jasongrout]

Like mentioned elsewhere, we will have problems if we have two different validators because validators also can coerce the argument to make it valid. Having the validator be a magic name enforces only one validator/coercer. Another option is to have the decorator fail the second time it is used to register a validator (I'm not sure if I like that any better, though...).

[SylvainCorlay]

A small detail: having an event attribute to the descriptor, later used by the common instance-init is not very nice IMO (switching behaviors based on the value of a string).
Since you use inheritance, you may as well specialize instance_init in both implementations of the descriptors, and remove the code handling the case of an invalid string.

[rmorshea]

@SylvainCorlay let me know if that last commit addresses your concern. @jasongrout I think I'm going to open up a new branch based on this one, that will try to address the organization that these decorators should take on (and hopefully in the process, solve the problem of chaining validators).

[SylvainCorlay]

inst.observe(method, name) ?

[rmorshea]

@SylvainCorlay or @jasongrout, are you guys available to chat briefly?

[SylvainCorlay]

typo ints

[jasongrout]

Let's not use the word "cross validate". How about "A decorator for validating the state of an HasTraits object when a Trait is set." or something like that.

[jasongrout]

We don't have chaining anymore, right?

[jasongrout]

Do we have tests to go along with this?

[rmorshea]

@jasongrout I was just using the same language as in 'observe', so I'll make that edit on both. And no tests yet, but I'll write those after lunch.

[jasongrout]

I don't think it makes sense to have a validator that takes no arguments. How will it know what value to return, if it has no idea what was set? I think validators should always take one argument, this change dictionary you created above.

[jasongrout]

@rmorshea - thanks. The observe should probably also use Trait instead of member. @SylvainCorlay, what do you think?

[rmorshea]

To the first, good point, on the other, I figured it would be best to prevent people from trying to pass in multiple attribute names, but since observe is *names they'd probably get an error for too many arguments instead. I'll take that out.

[SylvainCorlay]

This looks good to me.

[rmorshea]

I'm making these api updates to my matplotlib branch that uses traitlets and the callback signature of callback(self, change) is feeling a bit clunky because using change['name'] and change['new'] etc. is too verbose.

Because there isn't an easy way to enumerate the change dict (since it isn't ordered) I'm tempted to gravitate towards a naming scheme like callback(self, c) to clean that up, however c isn't particularly descriptive.

It might be nicer to make the signature callback(self, **change) for the cases where change['name'] isn't too bulky to use, and enumerating the dictionary into its corresponding variables is the only reasonable thing to do.

[SylvainCorlay]

I recall that we discussed this with Chris and Jason and the choice for not expanding the dictionary came from the fact that it is future-proof, in that we will be able to add extra arguments to observe in the future if needed. Chris might have had other reasons to do the same in Atom.

[rmorshea]

So then just go with callback(self, c) I guess?

[ssanderson]

@SylvainCorlay sadly (or maybe not...) that doesn't work in functions:

In [1]: def foo(x):
   ...:     locals().update(x)
   ...:     return y
   ...:

In [2]: foo({'y': 1})
---------------------------------------------------------------------------
NameError                                 Traceback (most recent call last)
<ipython-input-2-48bdc5fe6135> in <module>()
----> 1 foo({'y': 1})

<ipython-input-1-3cf55a7db847> in foo(x)
      1 def foo(x):
      2     locals().update(x)
----> 3     return y
      4

NameError: global name 'y' is not defined

It does work, however, in class bodies:

In [3]: class Foo(object):
   ...:     locals().update({'y': 1})
   ...:

In [4]: Foo().y
Out[4]: 1

[SylvainCorlay]

Sorry I deleted my previous message by mistake that was saying locals().update(change) 😄

[rmorshea]

So then we're back to callback(self, c)?

[SylvainCorlay]

I guess so. In any case, this PR is about the validation decorator. To change back to **change, we will need to get everyone around the table again on that decision...

[rmorshea]

old __call__ method forced the use of super to access overwritten methods. 9ea2afa enables this syntax:

from traitlets import HasTraits, Int, observe

class A(HasTraits):
    i, j = Int(), Int()
    @observe('i')
    def _i_changed(self, change):
        self.j = change['new']

class B(A):
    k = Int()
    @observe('i')
    def _i_changed(self, change):
        A._i_changed(self, change)
        self.k = change['new']

[ellisonbg]

I think it is a good idea for this to work with and without super, but let's add tests for that.

[SylvainCorlay]

@rmorshea I don't think that you need to create a MethodType in _func_call. You can simply call the unbound method since inst is passed as the first argument in *args. This would give the following for EventHandler:

class EventHandler(BaseDescriptor):

    def _init_call(self, func):
        self.func = func
        return self

    def __call__(self, *args, **kwargs):
        if hasattr(self, 'func'):
            return self.func(*args, **kwargs)
        else:
            return self._init_call(*args, **kwargs)

    def __get__(self, inst, cls=None):
        if inst is None:
            return self
        return types.MethodType(self.func, inst)

Besides, this would simplify the support of bare functions in the future.

[SylvainCorlay]

@minrk @jasongrout @ellisonbg do you guys want to have another look at this?

[minrk]

@SylvainCorlay thanks, I'll try to have a look tomorrow.

[rmorshea]

8ce0f31

• Corrects bad pickling test which didn't test for when methods are added to the instance as notifiers.
• Moves popping _notify_trait off __dict__ from the end of with_hold_trait_notifications to __getstate__ since the method needs to be there to handle the _trait_notifiers and _trait_validatators attributes anyway (consolidates pickling logic).
• Introduces __setstate__ to reassign notifiers attached to the HasTraits instance by EventHandler instances during instance_init. (this does not resolve methods which were assigned via inst.observe or inst.validate)

[SylvainCorlay]

@rmorshea let's hold off until this is merged and iterate in other PRs.

[rmorshea]

@SylvainCorlay I'll make a separate pr.

[minrk]

@SylvainCorlay @rmorshea 👍 from me. I'm happy to merge if this gets OK from @ellisonbg.

[ellisonbg]

Looks good, merging.