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

Add base and multi controllers #255

Conversation

CodyCBakerPhD
Copy link
Collaborator

This PR introduces more rigorously designed and tested infrastructure for the controllers introduced in #241

It consists of 3 classes:

BaseController
  • Serves as the 'abstract' representation of the class structure (though due to technical comments and discussion below was unable to use abc and instead raises NotImplementedErrors).
  • For a really custom controller (manual overrides of setup_components and setup_children), this is the class to inherit from.
GenericController
  • Defines a simple way of setting up components and children by default.
  • This is the most recommended type of controller to inherit from.
MultiController
  • Essentially a GenericController, but allows components to themselves be other controllers and has extra logic related to unpacking those controllers and validating field names.

I had previously thought to make these basic container classes that generate widget boxes via something like some_controller.make_box(box_type=ipywidgets.HBox). This would keep these classes more modular in scope and more generic with respect to the method of display which could allow easier generalization to other backends as suggested by @luiztauffer, but after playing around with it more I did find it more convenient to just make the base class inherit from ipywidgets.Box and treat the children as another attribute. This box also comes preset with the layout options consistently shared between HBox and VBox.

One question that remains is how to most conveniently allow swapping a controller (as a box) between HBox or VBox. By default it is an HBox, and all that is necessary to turn it into a VBox is the line some_controller.layout.flex_flow = 'column'. But I'd be open to making it a method of the class, such as some_controller.flip() or some_controller.set_orientation(orientation="vertical" or "horizontal").

@CodyCBakerPhD CodyCBakerPhD self-assigned this Jan 4, 2023
Comment on lines +72 to +82
button = Button()
check_box = Checkbox()
components = dict(
button=button,
check_box=check_box,
some_integer=1,
some_string="test",
some_float=1.23,
some_list=[1, 2, 3],
some_dict=dict(a=5, b=6, c=7),
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't really understand why GenericController contains non-widget components

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes it flexible for when you have multiple widget components that can mutually affect other properties - example from the original PR (not updated to this GenericController standard) is the RotationController: https://github.com/catalystneuro/nwbwidgets/blob/add_two_photon_color/nwbwidgets/controllers/image_controllers.py#L7-L29

Two buttons that affect a common value that is then used to determine some aspect of the final visualization.

Also, some attempts to use properties of widgets (an example being the fields of a DropDown) do not update in-place, so having a secondary attribute that is modifiable can be handy as well if you ever want interactions with those properties

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, but why throw all of these into one dictionary? I don't really understand the logic of this PR in several ways so let's talk about it when we meet tomorrow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, but why throw all of these into one dictionary?

For input, they naturally form key/value pairs as attributes. As far as why components itself is also saved as an attribute (which might seem redundant), yeah that's largely there for the MultiController which has the added behavior of more human-readable nesting of sub-controller components if someone ever wanted an easier direct reference to it as opposed to navigating lists of nested children. That is, some_multi_controller.components["SomeSubController"] instead of trying to find SomeSubController out of potentially several indices of some_multi_controller.children, which could get even worse when you start making MultiControllers out of MultiControllers.

I'd be OK with removing components as a stored attribute for the BaseController and the GenericController and only have it be present for the MultiController.

I don't really understand the logic of this PR in several ways so let's talk about it when we meet tomorrow.

I'd say it's better to ask as many questions as you can in text form so we have a permanent reference to look back on and also bring in @h-mayorquin and @luiztauffer who contributed a lot to early thoughts on designs for this kind of stuff

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand why you would want to gather all of the widgets into a single dictionary. I think that's a good idea. But why also put state attributes in there instead of just making them standard attributes of the class instance?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, good point. I can adjust it so that .components only stores ipywidgets object references. Other attributes can then be retrieved directly or via get_fields() (they convenience retrieval for seeing what attributes the class has instead of looking at it's magic __dict__)

@codecov
Copy link

codecov bot commented Jan 4, 2023

Codecov Report

Merging #255 (bfdec7e) into master (f9312f1) will increase coverage by 1.03%.
The diff coverage is 98.70%.

❗ Current head bfdec7e differs from pull request most recent head 64b712f. Consider uploading reports for the commit 64b712f to get more accurate results

@@            Coverage Diff             @@
##           master     #255      +/-   ##
==========================================
+ Coverage   66.27%   67.31%   +1.03%     
==========================================
  Files          35       38       +3     
  Lines        3443     3518      +75     
==========================================
+ Hits         2282     2368      +86     
+ Misses       1161     1150      -11     
Flag Coverage Δ
unittests 67.31% <98.70%> (+1.03%) ⬆️

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

Impacted Files Coverage Δ
nwbwidgets/controllers/multicontroller.py 97.14% <97.14%> (ø)
nwbwidgets/controllers/__init__.py 100.00% <100.00%> (ø)
nwbwidgets/controllers/basecontroller.py 100.00% <100.00%> (ø)
nwbwidgets/controllers/genericcontroller.py 100.00% <100.00%> (ø)

... and 13 files with indirect coverage changes

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

@bendichter
Copy link
Collaborator

bendichter commented Jan 5, 2023

Why the distinction between BaseController and GenericController? What is BaseController doing? Is any class other than GenericController inheriting from it? I see you are testing that is the tests but it is not clear to me what use-case you are simulating.

@CodyCBakerPhD
Copy link
Collaborator Author

Why the distinction between BaseController and GenericController? What is BaseController doing? Is any class other than GenericController inheriting from it? I see you are testing that is the tests but it is not clear to me what use-case you are simulating.

Mostly so we have a BaseController class references to use in the MRO so we can do things like isinstance(some_object, BaseController) which is only true if it inherits from the BaseController at some level.

Point there being that the GenericController is simply one 'hopefully' convenient way of helping someone define a controller (by simply passing 'components' that you want to set) but if anyone wanted to do the whole thing themselves in a custom manner they could do something like

def CustomController(BaseController):

    def __init__(self):
            # notice this is foregoing super().__init__()

            # normally stuff in setup_components()
            self.int_slider_1 = IntRangeSlider(...)
            self.dropdown = Dropdown(...)
            
            # normally in setup_observers()
            self.int_dropdown.observe(lambda change: self.update_range_slider_min_max(change.new))

            # normally in setup_children()
            self.children = (self.int_slider_1, self.dropdown)

and it would still qualify and behave exactly like something made via the GenericController process.

Now, if you think that's giving too much freedom and that it would be more beneficial to just mandate anyone making a Controller adhere to the structure of the GenericController, then I could very well shift the logic from the GenericController into the BaseController to make that so.

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.

2 participants