holoviz / param

Param: Make your Python code clearer and more reliable by declaring Parameters
https://param.holoviz.org
BSD 3-Clause "New" or "Revised" License
412 stars 69 forks source link

add_parameter and nested Parameterized #836

Closed samimia-swks closed 9 months ago

samimia-swks commented 9 months ago

ALL software version info

python 3.9.6 param 1.13.0

Description of expected behavior and the observed behavior

if I use .param.add_parameter() to add (nest) a param.ClassSelector(Parameterized, default=A()) parameter to a Parametrized class, all instances of B() seem to contain the same A() instance in that parameter.

In the output of the code below, I expect the last line to say b2.parameterized_dynamic.p_static = default_p_static Perhaps I am missing an instantiate or per_instance argument, but the help for add_paremeter() says Should result in a Parameter equivalent to one declared in the class's source code., and that is not what I see.

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

import param

class A(param.Parameterized):
    p_static = param.Parameter("default_p_static")

    def __init__(self, **params):
        super().__init__(**params)
        self.param.add_parameter("p_dynamic", param.Parameter("default_p_dynamic"))
a1 = A()
a2 = A()
a1.p_static = "new_p_static"
print("a1.p_static = " + a1.p_static)
print("a2.p_static = " + a2.p_static)
a1.p_dynamic = "new_p_dynamic"
print("a1.p_dynamic = " + a1.p_dynamic)
print("a2.p_dynamic = " + a2.p_dynamic)

class B(param.Parameterized):
    parameterized_static = param.ClassSelector(A, default=A())

    def __init__(self, **params):
        super().__init__(**params)
        self.param.add_parameter("parameterized_dynamic", param.ClassSelector(A, default=A()))
b1 = B()
b2 = B()
b1.parameterized_static.p_static = "new_p_static"
print("b1.parameterized_static.p_static = " + b1.parameterized_static.p_static)
print("b2.parameterized_static.p_static = " + b2.parameterized_static.p_static)
b1.parameterized_dynamic.p_static = "new_p_static"
print("b1.parameterized_dynamic.p_static = " + b1.parameterized_dynamic.p_static)
print("b2.parameterized_dynamic.p_static = " + b2.parameterized_dynamic.p_static)

Output

a1.p_static = new_p_static
a2.p_static = default_p_static
a1.p_dynamic = new_p_dynamic
a2.p_dynamic = default_p_dynamic
b1.parameterized_static.p_static = new_p_static
b2.parameterized_static.p_static = default_p_static
b1.parameterized_dynamic.p_static = new_p_static   
b2.parameterized_dynamic.p_static = new_p_static
maximlt commented 9 months ago

Hi @samimia-swks, I wouldn't be surprised to see bugs with add_parameter. Even if it's been available in the code base for a very long time, it's basically untested (I added a first simple test recently). However, running your code against the main branch I get this output which seems fine. I'd be nice to write a test that capture this behavior to avoid any potential regression.

a1.p_static = new_p_static
a2.p_static = default_p_static
a1.p_dynamic = new_p_dynamic
a2.p_dynamic = default_p_dynamic
b1.parameterized_static.p_static = new_p_static
b2.parameterized_static.p_static = default_p_static
b1.parameterized_dynamic.p_static = new_p_static
b2.parameterized_dynamic.p_static = default_p_static
samimia-swks commented 9 months ago

Thanks for the quick response. I tried v2.0.0a3 and got the same (correct) result as you, but when trying this version with my main code base, encountered an unrelated bug which I just filed (#837). I am going to see if I can get around this seperate issue and confirm that my original issue is solved.

jbednar commented 9 months ago

Note that Param 2 does have breaking changes; some previously accepted code will no longer be accepted in Param 2. But as the release notes will show, we think these changes are all improvements, and I hope you'll agree!

jbednar commented 9 months ago

I think this issue can be closed since the related issue has its own solution, but feel free to reopen if that's not the case!