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

Don't automatically set instantiate to True when constant is True #776

Merged
merged 8 commits into from
Jul 12, 2023

Conversation

maximlt
Copy link
Member

@maximlt maximlt commented Jun 29, 2023

Fixes #287

Pushing this WIP PR to get some feedback on the approach. The idea is that when:

  1. instantiate is True -> deepcopy default and assign it to the local cache (pseudo: parameterized_obj._X_param_value = deepcopy(parameter.default))
  2. constant is True -> don't deepcopy, just assign a reference to default to the local cache (pseudo: parameterized_obj._X_param_value = parameter.default)

With 1. taking precedence over 2.

The test added in 7635d55 and that is based on the one given in the issue passes with this change.

Of course this means that if the default object is mutable, any change made from a reference will be seen by all the other references, all pointing to the same updated object.

import param

class A(param.Parameterized):
    x = param.Number(0)

a = A(name="AAA")

class B(param.Parameterized):
    y = param.ObjectSelector(a, constant=True)

b = B(name="BBB")

B.y.x = 1
print(b.y.x)
# 0 before this PR
# 1 with this PR

b.y.x = 2
print(B.y.x)
# 1 before this PR
# 2 with this PR

If reviewers are OK with this approach, I'll need to clean up the code and do some refactoring, it's a bit hacky right now.

@maximlt maximlt requested a review from jbednar June 29, 2023 11:52
@maximlt maximlt requested a review from philippjfr July 10, 2023 08:47
Copy link
Member

@philippjfr philippjfr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. It's always very difficult to consider all the implications of these changes but let's just make sure we run all test suites against this and then merge.

@maximlt
Copy link
Member Author

maximlt commented Jul 12, 2023

The unit test suites of Panel, HoloViews and Lumen passed with this branch checked out. Merging.

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 this pull request may close these issues.

Why force instantiate for constant=True?
2 participants