param
param copied to clipboard
holoviz
Overriding parameter value with None in subclass ignored
This behavior just bit me. Anyone have a clue why this would be happening?
Test case:
import param
class A(param.Parameterized):
a = param.Number(default=10)
def __init__(self, **params):
super(A, self).__init__(**params)
print self.a
class B(A):
a = param.Number(default=None)
def __init__(self, **params):
super(B, self).__init__(**params)
print self.a
a=A()
b=B()
Output:
10
10
10
Ahh, seems this is actually expected behavior according to the docstrings.
Ordinarily, when a Python object is instantiated, attributes
not given values in the constructor will inherit the value
given in the object's class, or in its superclasses. For
Parameters owned by Parameterized classes, we have implemented
an additional level of default lookup, should this ordinary
lookup return only None.
In such a case, i.e. when no non-None value was found for a
Parameter by the usual inheritance mechanisms, we explicitly
look for Parameters with the same name in superclasses of this
Parameterized class, and use the first such value that we
find.
Not sure it is desirable though. Anyone have any suggestions how best to get the behavior I want. There should be some way of getting override with None behavior at least.
Inheritance of default values has nagged away at me. For example:
import
class X(param.Parameterized):
a = param.Number(default=5.0, bounds=(-10,10))
class Y(X):
a = param.Number(bounds=(0,10)) # someone just wants smaller bounds
>>> print Y().a
0.0
Would it make more sense for the default value of Y.a to be inherited from X.a than to come from param.Number.default?
Anyway, back to Philipp's problem. Could we change the logic of param inheritance for the default slot, so that instead of just looking for the first non-None value of default, we also check to see if the value of default is different from that of the parent class?
I don't understand -- why does Y.a not already inherit from x.a? I thought that was the crux of Philipp's problem, that the default value of None makes inheritance happen, but that he did not want that to happen in his case. I'm getting very confused here. Yes, we do want Y.a to inherit the value from X.a, even though it will cause Philipp's problem. I don't understand why Philipp has a problem if we aren't doing that now.
The inheritance mechanism assumes the default value of Parameter objects is None.
In Philipp's case, B.a inherits from A.a because B.a is a Number(Parameter) with a default of None (because a is created explicitly with default=None). In my example, Y.a does not inherit from X.a because Y.a is a Number(Parameter) with a default of 0.0 (which is not None); 0.0 is Number's default value for default.
The default slot not inheriting for any Parameter subclass with a default that is not None has bothered me for a long time but nobody has ever complained. (I assume I must be the only one who ever declares parameters without specifying default.)
I hope I'm saying this right...I'm very tired!
Ah. It sounds like we need to change all Parameters to have a default that is None? That will mean that if anyone does instantiate one of them with no default, that allow_None will become True for that parameter, but I guess if they haven't set any default it's poorly defined whether that is a bad thing or not. So I propose that we at least change Number to have a default of None, and probably change any other Parameter that does not have None as a default.
I don't see how your proposed solution to Philipp's problem will help. If X.a has a default of 5, and Y.a has no value specified and thus defaults to None (assuming we fix the above), then Y's default will be different from X's, and by your proposal would override X's value. But that's not normally what we want -- if Y has no default specified for a, Y.a should inherit from X.a, not get None. So I don't think that proposal will help. The only way I can see to fix Philipp's problem with our current architecture is adding an explicit extra slot/attribute saying that we really do want the default to be overridden with None in this specific case, and then we'd have to check that attribute on lookup. Seems like a big pain, and would bloat the slots with an extra slot that's almost never, ever used, so I'm not excited about doing that.
The only other approach I could see is to have some non-None default value that we create that is guaranteed not to match any real Parameter value. The crux of our problem is that we do want None to be an actual value sometimes, and not even all that rarely, yet we're also overloading it to mean 'not set'. I'm not sure if we can somehow create such a unique value, something like None or True or False that nothing can accidentally match. https://docs.python.org/2/library/constants.html does list NotImplemented as one of the few such values, and it actually might make sense to use that here. Or maybe we could define a special class, the way None seems to be defined, as "the sole value of types.NoneType"? NotImplemented doesn't sound crazy, the more I think about it, but I'm not certain it's ok...
In response to this issue initially, I first wrote a reply with two of your suggestions and a "sorry, not going to happen in time for you" resolution of the issue: either we need to explicitly record the fact that default has been set, or we need to use something like an Unset type to distinguish from None. Either of those things would be quite a big change. I'm not against something like Unset forever, but I'm probably against it right now. (Like you, I don't want to add handling for another slot recording that default's been set, so I'm against that proposal.) I didn't think about making all Parameter types have a default of None. That sounds like a way to fix the inheritance issue I brought up (which nobody has complained about), but won't fix Philipp's problem (which is the only thing anyone has actually complained about).
However, given the slot inheritance mechanism we currently have, the current defaults of Parameter classes, and if we assume that currently nobody declares a parameter without specifying default (i.e. effectively we have no inheritance of default values from Parameterized superclasses), why won't my small change to param inheritance for the default slot work for Philipp? Ignoring my X and Y classes, and just talking about A and B: param inheritance for B currently looks up the class hierarchy for the first non-None value of B.a's default slot, finds A.a's default of 10, and then sets B.a's default slot to 10. My suggestion is that instead, param inheritance for the default slot could first look to see if B.a's default slot is different from A.a's default slot; if it is different, no inheritance - otherwise, carry on as now. I was only suggesting this modification for the default slot, not the others, because we definitely do use inheritance for the other slots.
So I think my proposal is an easy, small change to make (a line or two) that would fix Philipp's problem. I was kind of assuming Philipp needs this in the upcoming param release for holoviews (and that the release should be soon!). However, the problem with my proposal is that if there are people out there who do not specify a default when declaring a parameter of a type that has default=None (e.g. Parameter itself), it will break their inheritance. So we probably can't make my suggested change (even though part of me is sure that there are no such people, since they'd also have to not be using Parameter types with non-None defaults, or else they'd have noticed the inheritance problem and would have complained...).
If there were no pressure for a release, fixing default inheritance properly in the same release as adding allow_None to all parameters would make sense to me, because the changes could all be described together. I guess my question is, does the holoviews release depend on fixing Philipp's issue, and is further delay in param's release ok - or can we get by with a release where this issue (and default inheritance in general) isn't fixed?
I've only skimmed this discussion but this idea crossed my mind before you mentioned it:
The only other approach I could see is to have some non-None default value that we create that is guaranteed not to match any real Parameter value.
It seems to me a lot of the problems come down to None being a valid Python value that you might want to set. In param, I already introduced a special value for param.Time, namely forever which is an instance of Infinity().
You could either use a singleton notset or create a very simple class that you can instantiate NotSet() to replace None as appropriate. I would go for the class approach as I think instantiation would not be a big overhead and would probably only be required when importing param.
Seems we all had the same thought, so it must be right :)
But my problem is that introducing something like NotSet/Unset and altering default values (as simple as that all seems) just before a release is a risky thing to do. So my question is: not fixing Philipp's problem before the upcoming param release - problem for the holoviews release, or not?
I think using something like NotSet or NotImplemented (which already exists and isn't that bad semantically) is the right way to go in the long run, and I don't even think it's likely to cause many problems other than possibly revealing hidden bugs we already have elsewhere, but I also don't think it's wise to do it right before a release. Right after is a good time, so that we can work with it a while and make sure we are happy with it. I don't think it would take long at all to make this change.
I don't think we should do Chris's simple fix, because it muddies the water about how inheritance works, probably temporarily, which could add just as many problems as it solves. I'd much rather make one change and be done. I think we should either do NotSet now, if we really need this functionality, or more likely, just work around Philipp's problem somehow temporarily and then revisit the issue after release. Philipp?
Right.
NotImplemented is a good choice - just make sure not to get confused with NotImplementedError! ;-p
Yes. But we probably should raise an error, probably an AttributeError rather than a NotImplementedError, for any Parameter for which the ultimate default value (after inheritance) was NotImplemented. I.e., it should not be ok to define a parameter as Number yet have it have a value returned as NotImplemented because no other default was defined. I presume this check would be achieved upon instantiation of a Parameterized, checking that for any Parameters that have a default of NotImplemented, there is a parent for which it is not NotImplemented. Otherwise people would be allowed to use NotImplemented like we currently use None, and we'd have the same problem all over again of being unable to distinguish between marking it for inheritance and actual intentional setting of that value.
I've just been bitten by this bug again! I would like to see it fixed soon.
Ok, I've fixed it. When you pull to test my fix, you should check out the new version of ParamOverrides too. I also cleaned up param's source code, finally! Oh and I've put in initial versions of Dimension and Notion into param too. Let me know what you think.
I know it is April Fool's Day but I would still like to hear you explain the Notion class to me. :-)
:)
Jokes aside, I am actually looking more generally at parameter attribute inheritance at the moment (albeit slowly, in my spare time). At the very least, it's poorly documented - but I think it's inconsistent.
Anyway, this will be the next thing I deal with in param (along with ParamOverrides).
While discussing #456 with @jbednar and @philippjfr we agreed that the PR there is a workaround but we didn't come to an agreement about the desired behavior. Philipp and I feel that having a parameter declaration inherit settings from a superclass is not necessarily safe, intuitive or desirable: perhaps instead of needing a special marker value distinct from None to say that a slot value should be inherited from a superclass, I would also consider not inheriting any settings at all.
To be clear, if you have parameter foo declared on class A (and B is a subclass of A), then I don't think a redeclaration of parameter foo on B should use any settings from the declaration on A.
I'm not sure what you two are proposing here. If we don't inherit settings across Parameter instances, how can we implement the behavior clearly documented on the Param homepage's very first example?
>>> class A(param.Parameterized):
... a = param.Number(0.5,bounds=(0,1),doc="Probability that...")
... b = param.Boolean(False,doc="Enable feature...")
>>> class B(A):
... b = param.Boolean(True)
Here class B has a different default value for parameter b than class A does, but it doesn't have to duplicate the docstring from the original declaration. That original declaration might be in some distant file or even a different module, and part of the whole point of Param is only having to document the parameter once (and maintain that doc, which could be multiple pages worth of text in a well-described system!) if the semantics don't change, and the same for bounds and other metadata about the parameter.
Are you really proposing that subclasses would need to duplicate docstrings, bounds, and all other metadata just to change any bit of the parameter's settings? If so, I would most strenuously object. Here, the issue is simply that we need a special value to indicate "not set" (and therefore inherited) that's distinct from the often-valid-as-a-parameter-value None), so that such inheritance can be done properly.
... if the semantics don't change, and the same for bounds and other metadata about the parameter.
That is exactly the point. In #456 the semantics did change as the parameter type also changed. The inheritance behavior is fine as long as the parameter type is fixed but when the parameter type changes I think inheritance is no longer appropriate.
My desired behavior would be to keep the current behavior (with the additional None/not set distinction) when the parameter type stays consistent but to stop inheritance when the overriding parameter type is different. I'm not sure this is possible from an implementation perspective though...
Ok, so you are revising your proposal to be more specific, i.e. no longer proposing to remove support for all inheritance of parameter attributes, but specifically to avoid inheriting in the one case where the parameter type changes between sub and superclass? In the more specific form I no longer strenuously object, and it does make sense that we can't be sure that semantics are preserved appropriately in that case, and thus punt to the user to force them to set the semantics they intend.
However, I don't see how that would help solve this issue. In #455 the type changed, but the type doesn't change in the example in this issue. We already have a solution for #455, not this one (#97), so I don't think we should introduce a breaking change to solve an already-addressed problem if we don't have a solution for this particular not-at-all addressed problem. I don't see any reason not to use the NotSet or NotImplemented approach outlined above, but someone would have to try it and see how it goes.
Ok, so you are revising your proposal to be more specific ...
Agreed.
I think the relevance of #455 is that the mechanism for inheritance (i.e supposing we have the distinction ofNotSet versus None) would ideally be disabled if the parameter type changed. I am presuming the mechanics of inheritance are currently the same whether or not the parameter type is changing, otherwise #455 wouldn't have been a problem to begin with.
In other words, NotSet can inherit a concrete value (including None) from a superclass but really this should only happen if the parameter types are the same (or at least very strictly compatible in a way we would need to define).
The way I'd define the compatibility is that subclasses should always satisfy the "is-a" relationship with the superclass. I.e., for an instance b of subclass B and an instance a of subclass A, b should always be usable where a is. That's the general rule for inheritance, and for Parameters it's specifically that any parameter value accepted by B should be a valid setting for A. That way any code written for A, including code that handles the various values of any attribute of A, can be trusted to work properly with any b.
We don't currently enforce this relationship, because as you've seen, a superclass can change the type of a Parameter, which for #455 was done in what I think is a compatible way (and thus not an error), yet in many cases it would be easy to violate the "is-a" relationship (e.g. changing from Integer to String). In #455 it was valid because the change was from two supported types down to one of the two supported types, which is fine for "is-a", but it's currently up to the programmer not to do an inappropriate type change, because it would be difficult for us to detect which type changes are valid and which are not. So I'm ok with disallowing inheritance in such a case, but it would be a breaking change and seems unlikely to come up often, so I wouldn't consider it a priority.
Note that I'm pretty sure we don't currently enforce "is-a" even in cases where it would be straightforward to do so and that don't involve changing types. E.g. if 'A' has an Integer parameter p and specifies an allowed range 0 to 10 and does not allow None, code written for A should be able to assume that p will never be outside that range and will never be None. Yet if subclass B changes the range of p to -20 to 20 and allows None, I'm pretty sure Param won't complain but any code written for A that doesn't expect negative numbers or None will balk when given a b with those values for p. If we are going to tighten up respecting "is-a", I'd argue we should at least warn in the case where someone defines a Parameter in a subclass that violates the bounds, allow_None, or similar assumptions that can be made of the superclass.
What you wrote makes sense though as you point out, it gets complicated if you want to enforce (or at least warn!) that the allowable parameter values for every child class should be a subset of what is allowed for the superclass.
So I'm ok with disallowing inheritance in such a case, but it would be a breaking change and seems unlikely to come up often, so I wouldn't consider it a priority.
Agreed though once again #455 doesn't seem that unreasonable or uncommon a thing to do (generalizing a base class in a compatible way). I'm fine with this suggestion being low priority in general but not for specific clashes like #455 where the resulting behavior appears buggy.
Generally, handling both typing and parameter settings to spot violations of the "is-a" property seems like a non-trivial task to do properly (and is orthogonal from simply distinguishing None from NotSet anyway).