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

Widgets are not unique when stored in a dict in sub-class #2944

Closed
aidanheerdegen opened this issue Aug 7, 2020 · 5 comments · Fixed by #3122
Closed

Widgets are not unique when stored in a dict in sub-class #2944

aidanheerdegen opened this issue Aug 7, 2020 · 5 comments · Fixed by #3122
Assignees
Milestone

Comments

@aidanheerdegen
Copy link

aidanheerdegen commented Aug 7, 2020

This may be a python feature rather than an ipywidgets bug, but thought it was worth pointing out in case someone else has a similar issue and can't figure it out.

I am subclassing HBox and VBox to create self-contained "combo-widgets". However, I noticed that when I made independent instances of my combo-widgets, the widgets they contained seemed to be connected. It turns out that because I was storing the widgets in a dict the widgets were pointing to the same address.

Here is an MCVE:

import ipywidgets as widgets
class myBox(widgets.HBox):
    def __init__(self):
        self.widgets['innerbox']=widgets.Text()
        super().__init__(children=[self.widgets['innerbox']])
b1 = myBox()
print(id(b1.widgets['innerbox']))
b2 = myBox()
print(id(b1.widgets['innerbox']), id(b2.widgets['innerbox']))

Result is

140583624309712
140583623982352 140583623982352

It doesn't happen if I store the inner widget like so

        self.innerbox=widgets.Text()

if I give the inner widgets different arguments every time they are instantiated they still end up pointing to the same place

import ipywidgets as widgets
import uuid
class myBox(widgets.HBox):
    def __init__(self):
        self.id = str(uuid.uuid1())
        self.widgets['innerbox']=widgets.Text(description=str(self.id))
        super().__init__(children=[self.widgets['innerbox']])
b1 = myBox()
print(b1.id, b1.widgets['innerbox'])
b2 = myBox()
print(b2.id, b2.widgets['innerbox'])
print(b1.id, b1.widgets['innerbox'])
print(id(b1.widgets['innerbox']), id(b2.widgets['innerbox']))
dfa1e66c-d865-11ea-bd85-fa163e8ac59c Text(value='', description='dfa1e66c-d865-11ea-bd85-fa163e8ac59c')
dfa587f4-d865-11ea-a034-fa163e8ac59c Text(value='', description='dfa587f4-d865-11ea-a034-fa163e8ac59c')
dfa1e66c-d865-11ea-bd85-fa163e8ac59c Text(value='', description='dfa587f4-d865-11ea-a034-fa163e8ac59c')
140583624267536 140583624267536
aidanheerdegen added a commit to COSIMA/cosima-cookbook that referenced this issue Aug 7, 2020
…ubwidgets

were pointed to the same memory location in different instants. For more
information see:

jupyter-widgets/ipywidgets#2944

Removed unused imports.
@ianhi
Copy link
Contributor

ianhi commented Aug 7, 2020

I think this is an issue of class instance variables vs class variables. I just read https://stackoverflow.com/questions/45284838/are-the-attributes-in-a-python-class-shared-or-not which I found to be helpful for understanding this.

I think this issue is in how you are (or are not) initializing self.widgets. If I change your example to this:

import ipywidgets as widgets
class myBox(widgets.HBox):
    def __init__(self):
        self.widgets = {}
        self.widgets['innerbox']=widgets.Text()
        super().__init__(children=[self.widgets['innerbox']])
b1 = myBox()
b2 = myBox()
print(id(b1.widgets['innerbox']) == id(b2.widgets['innerbox']))
print(b1.widgets is b2.widgets)

then I get the result I think you are expecting.

False
False

@ianhi
Copy link
Contributor

ianhi commented Aug 7, 2020

And I think the issue was coming up in this case because widgets is a class variable that is defined all the way back in the Widget base class

# widgets is a dictionary of all active widget objects
widgets = {}

So the solution is either to redefine it for the instance as I did, or use a different variable name.

@jasongrout
Copy link
Member

I think we should not have used such a common name, or figured out some other way of having that global list. That's a good thing to look at changing for ipywidgets 8.

@aidanheerdegen
Copy link
Author

Thanks for the quick feedback and fix.

My initial motivation for using a dict to store the widgets was for convenience, so I pass a list of all widgets to the super call with list(self.widgets.keys()), and to clearly identify which of the class variables belong to a widet. In practice I didn't use the first idiom that much, as there weren't that many cases where the widget layout was that clean. It also relied on implicit dict ordering, which is problematic.

As for identifying widgets, I could (didn't) use a convention of prepending w_ to all my widget names, or something similar.

In the end I just went with storing them all as unique class attributes/variables and it works fine. This was mostly to draw attention to what was quite a perplexing issue for me, in case someone else had the same (not)bright idea.

angus-g pushed a commit to COSIMA/cosima-cookbook that referenced this issue Aug 10, 2020
* Added data explore GUI

* Changed to local imports

* Fixed typo causing recursion

* Added explore to automatic imports

* Added some limited tests for explore.

* Added ipywidgets dependency to setup

* Cleaned up imports.

Removed redundant return_value_or_empty functions.

Fixed bug with ExperimentExplorer __init__ when session not passed.

* Fixed some code comments and user facing comments.

Changed to using a dict comprehension for formatting info output.

* Added minimal tests for ExperimentExplorer.

Modified test files for explore to ensure differences betweem two
experiments to make tests meaninful.

* Forgot to save last change when adding last tests

* Removed unused hvplot import

* Changed to storing widgets directly in attribute and not in dict as subwidgets
were pointed to the same memory location in different instants. For more
information see:

jupyter-widgets/ipywidgets#2944

Removed unused imports.

* White space cleanup.

Enforced consistent style across classes.
@jasongrout jasongrout added this to the 8.0 milestone Feb 2, 2021
@ianhi
Copy link
Contributor

ianhi commented Feb 16, 2021

Should this also be turned into a private attribute? My intuition is yes, but I can't find a firm reason either way.

Perhaps yes because otherwise if we rename it something like: active_widgets then it is weird for a slider to have active_widgets as an attribute when tab completing.

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

Successfully merging a pull request may close this issue.

3 participants