Skip to content
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

Setting default value of 'name' in a Parameterized class doesn't work #644

Closed
chbrandt opened this issue Jul 12, 2022 · 6 comments · Fixed by #740
Closed

Setting default value of 'name' in a Parameterized class doesn't work #644

chbrandt opened this issue Jul 12, 2022 · 6 comments · Fixed by #740
Assignees
Milestone

Comments

@chbrandt
Copy link

ALL software version info

channels:
  - conda-forge
  - defaults
dependencies:
  - ipykernel=6.15.1
  - ipython=8.4.0
  - jupyter_client=7.3.4
  - jupyter_core=4.10.0
  - libcxx=14.0.6
  - param=1.12.2
  - python=3.10.5
  - traitlets=5.3.0
  • MacOS 11.6.5

Description of expected behavior and the observed behavior

Setting a default (string) value to Parameterized class' name attribute doesn't work (as expected).
A given default value is overwritten by an incremental "Classname<SERIAL>" value.

We can set the/a value later, though.

I was expecting the default value to be used as initial value.

Complete, minimal, self-contained example code that reproduces the issue

class Person(param.Parameterized):
    name = param.String('ze')
    birthyear = param.Integer(1991)

p = Person()
p
# Person(birthyear=1991, name='Person00003')
p.name = 'ze'
p
# Person(birthyear=1991, name='ze')

Screenshots or screencasts of the bug in action

@hoxbro
Copy link
Member

hoxbro commented Jul 13, 2022

You shouldn't use the variable name as it is reserved. A more in-depth description is given here, which I have copied below.

  1. name is the one Parameter that Parameterized objects always have, and it’s also the only one that has special semantics in Param: If the name is not set on an instance or in its constructor, it has a name autogenerated for it from the class name. The intention of this mechanism is for each object to have some relatively unique name to distinguish it when exploring collections of objects, though that’s not necessarily always useful. In any case, here, if we want it to have the value John Doe as in the Pydantic example, we have to provide that name explicitly on the instance as above. The original Pydantic example doesn’t make a whole lot of sense for name anyway, as it would be a recipe for errors for something like a name to inherit a concrete value like John Doe from the base class.

@jbednar
Copy link
Member

jbednar commented Jul 22, 2022

I wonder if we could support this by changing Param's code, so that anyone who explicitly declares a name parameter thereby disables this "special semantics". The special semantics made sense at the time it was implemented, in the context of heavyweight objects that needed a unique visual representation as a string, but Parameterized objects are used in many other contexts nowadays, and it would be nice for people to be able to reject this special behavior and declare that name should behave just like any other parameter. The syntax above seems like a good way to declare that, but I'm not sure if we can make that work cleanly.

@chbrandt
Copy link
Author

chbrandt commented Aug 3, 2022

I agree with your suggestion @jbednar, if name is to keep being used as it is nowadays. I understand that would be a mid-way solution for something (the "special semantics") that has an ambiguous behaviour: at the end of the day, if the (name) attribute can be modified and its supposed uniqueness cannot be guaranteed (anymore), the "special semantics" mechanism behind it should be turned off anyway.

But I want to share/discuss something underneath it that is bouncing here in my head: I understand there is a design issue there. Bear with me, 'name' is a "public" member, a (fairly) common and meaningful word, that can be modified, holding (sometimes) a special meaning. From my understanding of OO, we have a broken encapsulation here. And from my understanding of Params, it blocks a very common attribute many users would use.

I know that would probably mean more work, but how about changing 'name' to, say, '__name' (or something alike, eg, '__id')? I emphasized "probably" before because I would not change any behaviour currently associated to 'name' (as suggested by you, @jbednar ), I would "just" change its label and decorate 'name' with a deprecation warning for all the current users out there.

Does that make sense?

@jbednar
Copy link
Member

jbednar commented Aug 3, 2022

if the (name) attribute can be modified and its supposed uniqueness cannot be guaranteed (anymore), the "special semantics" mechanism behind it should be turned off anyway.

Sorry if I gave the wrong impression: the name was never enforced or even assumed to be unique; it was only helpful that it was unique so that when the object's name was displayed (e.g. in an error message or as a representation of the object in a GUI) the user could distinguish this object from other objects.

What was enforced (and continues to be enforced, even with my proposed change) was that the name exists, so that the object would have some sort of label for use in those purposes. Users were always expected to update the name to something meaningful for them if they wanted the label to be useful, and the meaningful name has never been required to be unique. Given that the name is required to exist, the other reasonable default would have been to give it the name of its class, but doing that would make the visual representation ambiguous by default, which isn't desirable in a GUI context where people need to tell similar objects apart from each other.

So there hasn't been any change in this support or behavior (ever); the only difference is that Param has become used much more widely, outside of the context of a small number of heavyweight objects a user might want to manipulate in a GUI context or visually represent in text reports.

'name' is a "public" member, a (fairly) common and meaningful word, that can be modified, holding (sometimes) a special meaning.

I think I gave the wrong impression here, by not making a distinction between the two different ways that name parameter has special significance: (1) the actual code that ensures the name always exists and generates unique values for it by default unless overridden in the instance, and (2) the convention that other libraries like Panel and some parts of Param itself treat the name parameter as a label for the object.

I'm proposing that the behavior in (1) be configurable; if people don't want special auto-generated names, there's no reason they should have to have them! Param doesn't require that the names be unique, and people should be able to use any non-unique name they feel like, instead of having to fight Param. Making this change shouldn't affect any other behavior, as it does not change the semantics of name in the constructed object.

Having relaxed (1), it's up to the user what they do about (2). Wherever name is used as a default label, e.g. in Selectors, you should always be able to provide an alternative label, e.g. by supplying a dictionary to a Selector rather than a list of named objects. Here if people want to build a system that makes no assumption about name even existing or being a suitable label, they can do so; Param shouldn't care that you do that. But tooling around Param, such as Panel, does make an assumption that it can use the name as a default label, and I'm not proposing that Panel change that.

how about changing 'name' to, say, '__name' (or something alike, eg, '__id')?

To be clear, name is not an id (which people will assume is unique), it is a label for the object when displayed. I could imagine adding a special __label field that would override the name for display purposes, but then that would have to be respected by libraries using Param, which seems painful to add at this point.

@chbrandt
Copy link
Author

chbrandt commented Aug 3, 2022

Perfect, I now understand the whole thing. It was my mistake thinking it should be unique. After that, I would say everything is indeed the way it should be. I'm closing this. Thanks.

@chbrandt chbrandt closed this as completed Aug 3, 2022
@jbednar
Copy link
Member

jbednar commented Aug 3, 2022

I'd still like to address your original concern, namely (no pun intended) that your user-supplied "ze" name was not respected as the default value for this parameter. That's about behavior (1), which I think we can and should change. If a user has specified some particular value for name explicitly in a Parameterized class, I think Param should respect it, and I think we can do that without disrupting any other behavior of Param.

I briefly tried to do that by suppressing the self.param._generate_name() call in the Parameterized constructor, but it looks like it will be tricky to implement until #605 is merged, which is on my plate to finish up and merge anyway. So I'd like to reopen this and revisit it after parameters allow sentinel values, so that we can detect that the user has set the value explicitly and disable all auto-generating functionality in that case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants