-
-
Notifications
You must be signed in to change notification settings - Fork 74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Overriding parameter value with None in subclass ignored #97
Comments
Ahh, seems this is actually expected behavior according to the docstrings.
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 Anyway, back to Philipp's problem. Could we change the logic of param inheritance for the |
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 In Philipp's case, The 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 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 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 If there were no pressure for a release, fixing |
I've only skimmed this discussion but this idea crossed my mind before you mentioned it:
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 You could either use a singleton |
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.
|
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 |
:) 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). |
Ouch. I was prepared to believe everything except Notion. :-) |
Did this end up in #113? |
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 |
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?
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. |
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. |
Agreed. I think the relevance of #455 is that the mechanism for inheritance (i.e supposing we have the distinction of In other words, |
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 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 |
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.
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 |
This behavior just bit me. Anyone have a clue why this would be happening?
Test case:
Output:
The text was updated successfully, but these errors were encountered: