Skip to content

Fix copying when having watchers on e.g. bounds on inherited Parameter types #675

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

Merged
merged 2 commits into from
Jan 6, 2023

Conversation

ovidner
Copy link
Contributor

@ovidner ovidner commented Jan 5, 2023

When exposing a parameter with Panel, it might be watched for changes in its bounds. Suppose you later want to make a deepcopy of this parameter. This will work fine if the parameter is a Number, but not if it is e.g. an Integer, per this example:

import param
import copy


class A(param.Parameterized):
    n = param.Number(default=0)
    i = param.Integer(default=0)


obj_1 = A()
obj_1.param.watch(lambda x: x, 'n', 'bounds')
copy.deepcopy(obj_1)  # Works!

obj_2 = A()
obj_2.param.watch(lambda x: x, 'i', 'bounds')
copy.deepcopy(obj_2)  # Raises AttributeError!

As I see it, this issue is due to Parameter.__setattr__ only checking for slot attributes in the class in question itself, and not its superclasses:

slot_attribute = attribute in self.__slots__

In the case of Integer, the bounds attribute is part of __slots__ in Number, but not in Integer, later resulting in the error above.

Proposed solution

Use get_all_slots to traverse up the class hierarchy and see if the attribute is part of any superclass' __slots__.

This is the shortest fix I could think of, but should this rather be handled by calling the superclasses' __setattr__ a few rows down?

super(Parameter, self).__setattr__(attribute, value)

I know too little about the nuances of Param to tell. However, the approach does not seem to break any tests.

I would love to have this solved, so please chime in on how we can do it the best way possible 😃

@philippjfr
Copy link
Member

The reasoning and the fix make sense to me. @jbednar could you also take a quick look?

Copy link
Member

@jbednar jbednar left a comment

Choose a reason for hiding this comment

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

Looks like the right fix to me, thanks!

@philippjfr philippjfr merged commit 284d594 into holoviz:main Jan 6, 2023
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.

3 participants