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

Fix default vdims in raster.RGB (round two) #5775

Merged
merged 1 commit into from
Jun 22, 2023
Merged

Fix default vdims in raster.RGB (round two) #5775

merged 1 commit into from
Jun 22, 2023

Conversation

hoxbro
Copy link
Member

@hoxbro hoxbro commented Jun 21, 2023

Follow up to #5773

This is how RGB worked before:

import param

class Dim: pass

class RGB(param.Parameterized):
    value = param.List(default=[Dim(), Dim()])

    def __init__(self, value=None, **params):
        if value is None:
            value = list(self.value)

        super().__init__(value=value, **params)

So if Example() was run, value would use a copy of the self.value with the default values in it. So running something like this would only change the id of the list, but not of the values inside of the list:

image

By changing value = list(self.value) to value = copy.deepcopy(self.value), we get the expected behavior, a unique ID for everything.

image

And if we set value = self.value, we also get the same identity for the list, which is something we really do not want.
image

Because we are setting the value directly before calling super(), we are bypassing Param's mechanism for deep copying the default values.

@hoxbro hoxbro requested a review from maximlt June 21, 2023 20:22
@codecov-commenter
Copy link

codecov-commenter commented Jun 21, 2023

Codecov Report

Merging #5775 (2cd76d9) into main (be58b7e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #5775   +/-   ##
=======================================
  Coverage   88.16%   88.17%           
=======================================
  Files         307      307           
  Lines       62839    62850   +11     
=======================================
+ Hits        55405    55416   +11     
  Misses       7434     7434           
Flag Coverage Δ
ui-tests 22.41% <38.46%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
holoviews/element/raster.py 78.97% <100.00%> (+0.04%) ⬆️
holoviews/tests/element/test_raster.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@maximlt maximlt left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix, it looks good to me.

I find this Param behavior pretty error prone and I wouldn't be surprised to find other similar bugs through the code base.

The __init__ below looks like a no-op but effectively overrides the deepcopied list (yep it's deepcopied anyway which isn't super efficient):

import param

class Foo(): pass

L = [Foo(), Foo(), Foo()]

class P(param.Parameterized):
    l = param.List(default=L)

    def __init__(self, l=None, **params):
        if l is None:
            l = self.l
        super().__init__(l=l, **params)

p = P()

assert P.l is not p.l  # Fails :(

Another way to fix that kind of thing is to avoid passing the list up to the super class if it's not modified, not to confuse Param's machinery, with something like that:

import param

class Foo(): pass

L = [Foo(), Foo(), Foo()]

class P(param.Parameterized):
    l = param.List(default=L)

    def __init__(self, l=None, **params):
        if l is None:
            # l = list(self.l)
            l = self.l
        else:
            l = list(l) if isinstance(l, list) else [l]
        if l is not self.l:
            params['l'] = l
        super().__init__(**params)

p = P()

assert P.l is not p.l  # Passes
assert all(a is not b for a, b in zip(P.l, p.l))  # Passes

I don't think there is much we can do at the Param level, besides documenting this behavior (it could avoid the deepcopy, maybe).

holoviews/element/raster.py Show resolved Hide resolved
@hoxbro hoxbro merged commit 00e64f3 into main Jun 22, 2023
14 checks passed
@hoxbro hoxbro deleted the fix_vdims_raster2 branch June 22, 2023 11:23
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