Refactor TwoPhotonSeries #241
Conversation
| # TOD: Figure out formula for calculating by in one-shot | ||
| by_width = 2 | ||
| by_height = 2 |
There was a problem hiding this comment.
Still a TODO item
| path_ext_file = indexed_timeseries.external_file[0] | ||
| # Get Frames dimensions | ||
| tif = TiffFile(path_ext_file) | ||
| n_samples = len(tif.pages) | ||
| page = tif.pages[0] | ||
|
|
||
| def _add_fig_trace(img_fig: go.Figure, index): | ||
| if self.figure is None: | ||
| self.figure = go.FigureWidget(img_fig) | ||
| else: | ||
| self.figure.for_each_trace(lambda trace: trace.update(img_fig.data[0])) | ||
|
|
||
| def update_figure(index=0): | ||
| # Read first frame | ||
| img_fig = px.imshow(imread(path_ext_file, key=int(index)), binary_string=True) | ||
| _add_fig_trace(img_fig, index) | ||
|
|
||
| self.slider = widgets.IntSlider( | ||
| value=0, min=0, max=n_samples - 1, orientation="horizontal", description="TIFF index: " | ||
| ) | ||
| self.controls = dict(slider=self.slider) | ||
| self.slider.observe(lambda change: update_figure(index=change.new), names="value") | ||
|
|
||
| update_figure() | ||
|
|
||
| series_name = indexed_timeseries.name | ||
| base_title = f"TwoPhotonSeries: {series_name}" | ||
| self.figure.layout.title = f"{base_title} - read from first external file" | ||
|
|
||
| self.children = [self.figure, self.slider] |
There was a problem hiding this comment.
TBH this entire section could also be absorbed into a minimal child class (the only thing different is where/how the data is fetched...
Might prefer to do in separate PR though, this is more than enough as is.
| """The rotation attribute of the SinglePlaneDataController cannot be attached in a modifiable state.""" | ||
| if not hasattr(self, "Controller"): # First time this is called | ||
| return 0 | ||
| return self.Controller.components[self.data_controller_name].components["RotationController"].rotation |
There was a problem hiding this comment.
This is the only time a reference to a nested child is necessary (due to attribute mutability relative to the button observer events)
Technically I could override those observers and remap it all to an attribute specific to these classes, but that ends up taking many more lines of code than this simple router
That said, I built the MutliController to at least have dictionary mapping capability, so you can navigate it entirely based on name references to the components (much easier than children indexes)
| class MultiController(widgets.VBox): | ||
| controller_fields: Tuple[str] = tuple() | ||
| components: Dict[str, widgets.VBox] = dict() | ||
|
|
||
| def __init__(self, components: list): | ||
| super().__init__() | ||
|
|
||
| children = list() | ||
| controller_fields = list() | ||
| self.components = {component.__class__.__name__: component for component in components} | ||
| for component in self.components.values(): | ||
| # Set attributes at outermost level | ||
| for field in component.controller_fields: | ||
| controller_fields.append(field) | ||
| setattr(self, field, getattr(component, field)) | ||
|
|
||
| # Default layout of children | ||
| if isinstance(component, widgets.Widget) and not isinstance(component, MultiController): | ||
| children.append(component) | ||
|
|
||
| self.children = tuple(children) | ||
| self.controller_fields = tuple(controller_fields) | ||
|
|
||
| self.setup_observers() | ||
|
|
||
| def setup_observers(self): | ||
| pass |
There was a problem hiding this comment.
I don't like the distinction between Controllers and MultiControllers. I think a widget that contains multiple controllers should just be another controller.
There was a problem hiding this comment.
It's fine to have a MultiController class that automates combination of Controllers. But this class should also be a Controller.
There was a problem hiding this comment.
Yeah, I just need to utilize a base Controller class, which a MultiController itself is as well in every respect
| from ..controllers import RotationController, ImShowController, ViewTypeController, MultiController | ||
|
|
||
|
|
||
| class FrameController(widgets.VBox): |
There was a problem hiding this comment.
There should be a Controller base class.
| self.children = (self.frame_slider,) | ||
|
|
||
|
|
||
| class PlaneController(widgets.VBox): |
There was a problem hiding this comment.
I think defining controllers that contain a single widget is a bit overkill. In my mind, the point of controllers is collections of widgets (or collections of collections of widgets) in a system that is reusable.
| import ipywidgets as widgets | ||
|
|
||
|
|
||
| class MultiController(widgets.VBox): |
There was a problem hiding this comment.
Can you make this inherit from widgets.Box so components can be either horizontal or vertical? See
https://ipywidgets.readthedocs.io/en/7.x/examples/Widget%20Styling.html#The-VBox-and-HBox-helpers
There was a problem hiding this comment.
Yes I started running into this on another PR as well
|
Going to modularize certain aspects of the PR, like the controllers, into separate sub-PRs. Moving to draft for now |
Inspired to test the proposal structure of #239, and replacing #230, I refactored the
ophyssubmodule to follow the proposed workflow to show how much more readable the code is, as well as how well it can generalize to allow easy creation of subwidgets that differ only slightly in how they setup different parts of the total visualization.The TwoPhotonSeries visualizations are a good case to use for this because they use essentially the same types of plots for planar view of both single-plane instances (3D data) and volumetric cases (4D data). Thus if any improvements are ever made to either plot, if they didn't have some type of inheritance structure those improvements would have to be duplicated in code. Also we have several project going on right now and coming up (looking at MICrONS) that use these modalities, so further enhancements are no doubt to come.
Several such improvements have been made here...
plotly.express.imshowBoth the
SinglePlaneVisualizationand thePlaneSliceVisualizationshare these new features - the only difference between them is the latter has a single extra controller to allow specification of the plane index. As such, the inherited class does as little as possible to produce the key differences.I also tried to keep all controllers here as general as possible; only the final assembly-level ones are kept in the
ophys_controllers.pyas being specifically designed for use the otherophysclasses. This includes the controllers which have multiple components and their own internally interacting observers, which is one of the design proposals from @h-mayorquin that would up helping a lot to reduce complexity.Another really helpful suggestion from @h-mayorquin was the
MultiControllerclass for assembling these components and allowing an outer exposure of the attributes so we don't have to meticulously (and painfully, sometimes) recurse down through the levels ofipywidgets.Box.childrento find an object we want to reference. It also makes these references more verbose, and thus the code more readable.I also re-organized the file structure to be more readable/navigable instead of everything being in a single file. When you start piling on more visualization for neurodata types from a single modality like that, it gets hard to scroll through looking for items you might want to find; even ctrl-F or GitHub link navigation doesn't always help much.
If this is too much to review all at once, I'd be happy to break the individual controllers into modular PRs accompanied by their own tests and even documentation. Figured it might be nice to see an explicit example of proposal structure #239 in practice and how it can help us keep this project more easily manageable going forward.
Video demonstration:
video2227929494.mp4
TODO: