-
Notifications
You must be signed in to change notification settings - Fork 52
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
[MRG] Update GUI to latest version of ipywidgets and voila #696
Changes from 25 commits
be01e85
266e789
bf70536
c10b708
d3ec97e
a7786ff
e81d247
94820fa
7ce1404
b7d45e9
bf3580c
0c0e877
a629787
4391b46
94576d9
a8e9ebb
c3a805b
261a025
ecd2e0e
c882122
1a9b287
01c758a
1dd4568
b54b656
e14da5f
2c38ba3
f91cbb2
96e4a6a
d49cad3
207de0c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ | |
|
||
import copy | ||
import io | ||
from functools import partial | ||
from functools import partial, wraps | ||
|
||
import matplotlib | ||
import matplotlib.pyplot as plt | ||
|
@@ -99,6 +99,35 @@ def plot_type_coupled_change(new_plot_type, target_data_selection): | |
target_data_selection.disabled = False | ||
|
||
|
||
def unlink_relink(attribute: str): | ||
""" | ||
Wrapper function to unlink widgets to perform edits and re-link them on | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check PEP 257 convention ... need a one-line description first ... longer description may follow later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Wrapper function" as in decorator, right? |
||
completion. Used as a decorator on class methods. The class must have an | ||
attribute containing an ipywidgets/traitlets link object. | ||
|
||
Parameters | ||
---------- | ||
attribute: The class attribute containing link object of ipywidgets widgets | ||
|
||
""" | ||
def _unlink_relink(f): | ||
@wraps(f) | ||
def wrapper(self, *args, **kwargs): | ||
# Unlink the widgets using the provided link object | ||
link_attribute: link = getattr(self, attribute) | ||
link_attribute.unlink() | ||
|
||
# Call the original function | ||
result = f(self, *args, **kwargs) | ||
|
||
# Re-link the widgets using link.link() | ||
link_attribute.link() | ||
|
||
return result | ||
return wrapper | ||
return _unlink_relink | ||
|
||
|
||
def _idx2figname(idx): | ||
return f"Figure {idx}" | ||
|
||
|
@@ -150,7 +179,7 @@ def _update_ax(fig, ax, single_simulation, sim_name, plot_type, plot_config): | |
|
||
elif plot_type == 'PSD': | ||
if len(dpls_copied) > 0: | ||
color = next(ax._get_lines.prop_cycler)['color'] | ||
color = ax._get_lines.get_next_color() | ||
dpls_copied[0].plot_psd(fmin=0, fmax=50, ax=ax, color=color, | ||
label=sim_name, show=False) | ||
|
||
|
@@ -175,7 +204,7 @@ def _update_ax(fig, ax, single_simulation, sim_name, plot_type, plot_config): | |
else: | ||
label = sim_name | ||
|
||
color = next(ax._get_lines.prop_cycler)['color'] | ||
color = ax._get_lines.get_next_color() | ||
if plot_type == 'current dipole': | ||
plot_dipole(dpls_copied, | ||
ax=ax, | ||
|
@@ -226,9 +255,7 @@ def _update_ax(fig, ax, single_simulation, sim_name, plot_type, plot_config): | |
def _static_rerender(widgets, fig, fig_idx): | ||
logger.debug('_static_re_render is called') | ||
figs_tabs = widgets['figs_tabs'] | ||
titles = [ | ||
figs_tabs.get_title(idx) for idx in range(len(figs_tabs.children)) | ||
] | ||
titles = figs_tabs.titles | ||
fig_tab_idx = titles.index(_idx2figname(fig_idx)) | ||
fig_output = widgets['figs_tabs'].children[fig_tab_idx] | ||
fig_output.clear_output() | ||
|
@@ -501,18 +528,25 @@ def _on_plot_type_change(new_plot_type): | |
def _close_figure(b, widgets, data, fig_idx): | ||
fig_related_widgets = [widgets['figs_tabs'], widgets['axes_config_tabs']] | ||
for w_idx, tab in enumerate(fig_related_widgets): | ||
# Get tab object's list of children and their titles | ||
tab_children = list(tab.children) | ||
titles = [tab.get_title(idx) for idx in range(len(tab.children))] | ||
titles = list(tab.titles) | ||
# Get the index based on the title | ||
tab_idx = titles.index(_idx2figname(fig_idx)) | ||
# Remove the child and title specified | ||
print(f"Del fig_idx={fig_idx}, fig_idx={fig_idx}") | ||
del tab_children[tab_idx], titles[tab_idx] | ||
|
||
tab.children = tuple(tab_children) | ||
[tab.set_title(idx, title) for idx, title in enumerate(titles)] | ||
tab_children.pop(tab_idx) | ||
titles.pop(tab_idx) | ||
# Reset children and titles of the tab object | ||
tab.children = tab_children | ||
tab.titles = titles | ||
|
||
# If the figure tab group... | ||
if w_idx == 0: | ||
# Close figure and delete the data | ||
plt.close(data['figs'][fig_idx]) | ||
del data['figs'][fig_idx] | ||
data['figs'].pop(fig_idx) | ||
# Redisplay the remaining children | ||
n_tabs = len(tab.children) | ||
for idx in range(n_tabs): | ||
_fig_idx = _figname2idx(tab.get_title(idx)) | ||
|
@@ -522,10 +556,11 @@ def _close_figure(b, widgets, data, fig_idx): | |
with tab.children[idx]: | ||
display(data['figs'][_fig_idx].canvas) | ||
|
||
if n_tabs == 0: | ||
widgets['figs_output'].clear_output() | ||
with widgets['figs_output']: | ||
display(Label(_fig_placeholder)) | ||
# If all children have been deleted display the placeholder | ||
if n_tabs == 0: | ||
widgets['figs_output'].clear_output() | ||
with widgets['figs_output']: | ||
display(Label(_fig_placeholder)) | ||
|
||
|
||
def _add_axes_controls(widgets, data, fig, axd): | ||
|
@@ -565,8 +600,8 @@ def _add_figure(b, widgets, data, scale=0.95, dpi=96): | |
with widgets['figs_output']: | ||
display(widgets['figs_tabs']) | ||
|
||
widgets['figs_tabs'].children = widgets['figs_tabs'].children + ( | ||
fig_outputs, ) | ||
widgets['figs_tabs'].children = \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. backslashes are not preferred ... would suggest using paranthesis if possible |
||
[s for s in widgets['figs_tabs'].children] + [fig_outputs] | ||
widgets['figs_tabs'].set_title(n_tabs, _idx2figname(fig_idx)) | ||
|
||
with fig_outputs: | ||
|
@@ -627,7 +662,7 @@ def __init__(self, gui_data, viz_layout): | |
self.figs_tabs = Tab() | ||
self.axes_config_tabs.selected_index = None | ||
self.figs_tabs.selected_index = None | ||
link( | ||
self.figs_config_tab_link = link( | ||
(self.axes_config_tabs, 'selected_index'), | ||
(self.figs_tabs, 'selected_index'), | ||
) | ||
|
@@ -711,6 +746,7 @@ def compose(self): | |
]) | ||
return config_panel, fig_output_container | ||
|
||
@unlink_relink(attribute='figs_config_tab_link') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need to think of one line comment on ipywidgets figure linking logic There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps: "Unlinks Tab objects to avoid index error from asynchronous addition of new tabs." There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry I missed the discussion on this, is it a bugfix or something related to the new version of ipywidgets? |
||
def add_figure(self, b=None): | ||
"""Add a figure and corresponding config tabs to the dashboard. | ||
""" | ||
|
@@ -729,7 +765,7 @@ def _simulate_switch_fig_template(self, template_name): | |
|
||
def _simulate_delete_figure(self, fig_name): | ||
tab = self.axes_config_tabs | ||
titles = [tab.get_title(idx) for idx in range(len(tab.children))] | ||
titles = tab.titles | ||
assert fig_name in titles | ||
tab_idx = titles.index(fig_name) | ||
|
||
|
@@ -764,16 +800,13 @@ def _simulate_edit_figure(self, fig_name, ax_name, simulation_name, | |
assert operation in ("plot", "clear") | ||
|
||
tab = self.axes_config_tabs | ||
titles = [tab.get_title(idx) for idx in range(len(tab.children))] | ||
titles = tab.titles | ||
assert fig_name in titles, "No such figure" | ||
tab_idx = titles.index(fig_name) | ||
self.axes_config_tabs.selected_index = tab_idx | ||
|
||
ax_control_tabs = self.axes_config_tabs.children[tab_idx].children[1] | ||
ax_titles = [ | ||
ax_control_tabs.get_title(idx) | ||
for idx in range(len(ax_control_tabs.children)) | ||
] | ||
ax_titles = ax_control_tabs.titles | ||
assert ax_name in ax_titles, "No such axis" | ||
ax_idx = ax_titles.index(ax_name) | ||
ax_control_tabs.selected_index = ax_idx | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,14 +3,18 @@ | |
import matplotlib.pyplot as plt | ||
import numpy as np | ||
import pytest | ||
import traitlets | ||
|
||
from hnn_core import Dipole, Network, Params | ||
from hnn_core.gui import HNNGUI | ||
from hnn_core.gui._viz_manager import _idx2figname, _no_overlay_plot_types | ||
from hnn_core.gui._viz_manager import _idx2figname, _no_overlay_plot_types, \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. paranthesis instead of backslash |
||
unlink_relink | ||
from hnn_core.gui.gui import _init_network_from_widgets | ||
from hnn_core.network import pick_connection | ||
from hnn_core.network_models import jones_2009_model | ||
from hnn_core.parallel_backends import requires_mpi4py, requires_psutil | ||
from IPython.display import IFrame | ||
from ipywidgets import Tab, Text, link | ||
|
||
matplotlib.use('agg') | ||
|
||
|
@@ -413,3 +417,50 @@ def test_gui_adaptive_spectrogram(): | |
for attr in dir(gui.viz_manager.figs[figid])]) is False | ||
assert len(gui.viz_manager.figs[1].axes) == 2 | ||
plt.close('all') | ||
|
||
|
||
def test_unlink_relink_widget(): | ||
"""Tests the unlinking and relinking of widgets decorator.""" | ||
|
||
# Create a basic version of the VizManager class | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's mostly out of principle to make the test small and clear so that the scope of the test is just the functionality of the decorator, which makes it easier to detect if the a bug is coming from within the function vs externally. The full functionality tests of the VizManager like |
||
class MiniViz: | ||
def __init__(self): | ||
self.tab_group_1 = Tab() | ||
self.tab_group_2 = Tab() | ||
self.tab_link = link( | ||
(self.tab_group_1, 'selected_index'), | ||
(self.tab_group_2, 'selected_index'), | ||
) | ||
|
||
def add_child(self, to_add=1): | ||
n_tabs = len(self.tab_group_2.children) + to_add | ||
# Add figure tab and select latest tab | ||
self.tab_group_1.children = \ | ||
[Text(f'Test{s}') for s in np.arange(n_tabs)] | ||
self.tab_group_1.selected_index = n_tabs - 1 | ||
|
||
self.tab_group_2.children = \ | ||
[Text(f'Test{s}') for s in np.arange(n_tabs)] | ||
self.tab_group_2.selected_index = n_tabs - 1 | ||
|
||
@unlink_relink(attribute='tab_link') | ||
def add_child_decorated(self, to_add): | ||
self.add_child(to_add) | ||
|
||
# Check that widgets are linked. | ||
# Error from tab groups momentarily having a different number of children | ||
gui = MiniViz() | ||
with pytest.raises(traitlets.TraitError, match='.*index out of bounds.*'): | ||
gui.add_child(2) | ||
|
||
# Check decorator unlinks and is able to make a change | ||
gui = MiniViz() | ||
gui.add_child_decorated(2) | ||
assert len(gui.tab_group_1.children) == 2 | ||
assert gui.tab_group_1.selected_index == 1 | ||
assert len(gui.tab_group_2.children) == 2 | ||
assert gui.tab_group_2.selected_index == 1 | ||
|
||
# Check if the widgets are relinked, the selected index should be synced | ||
gui.tab_group_1.selected_index = 0 | ||
assert gui.tab_group_2.selected_index == 0 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -106,7 +106,7 @@ def run(self): | |
'h5io' | ||
], | ||
extras_require={ | ||
'gui': ['ipywidgets <=7.7.1', 'ipympl<0.9', 'voila<=0.3.6'], | ||
'gui': ['ipywidgets>=8.0.0', 'ipykernel', 'ipympl', 'voila'], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you'll need to update README: https://jonescompneurolab.github.io/hnn-core/stable/index.html let me know once you fix it and I'll go ahead and merge @gtdang ! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! Updated! |
||
'opt': ['scikit-learn'] | ||
}, | ||
python_requires='>=3.8', | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't do type annotations ... I think it will have to be a repo-wide change, let's stick to repo conventions for now to keep things consistent