-
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] Fix dipole plot scale and smooth factors #730
Changes from 7 commits
5f235be
95ed9c1
3ac1386
ccb255a
d480947
affbfda
e47a1e8
e3ae1c4
c4f2319
3be7a2d
7de6d8f
2dcf972
e83b008
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 |
---|---|---|
|
@@ -17,6 +17,7 @@ | |
from hnn_core.gui._logging import logger | ||
from hnn_core.viz import plot_dipole | ||
|
||
|
||
_fig_placeholder = 'Run simulation to add figures here.' | ||
|
||
_plot_types = [ | ||
|
@@ -116,17 +117,21 @@ def unlink_relink(attribute): | |
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() | ||
try: | ||
# 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) | ||
# Call the original function | ||
result = f(self, *args, **kwargs) | ||
|
||
# Re-link the widgets | ||
link_attribute.link() | ||
# Re-link the widgets | ||
link_attribute.link() | ||
|
||
return result | ||
return result | ||
except Exception as e: | ||
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. Usually you specify what kind of exception you are trying to catch and handle here. Is there a specific exception that you are encountering? 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. Some GUI exceptions arent catch, like when out of bound or when it tries to use variables that haven't been set up yet. 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 see. If this is a good way for us to debug during development let's just keep this in for now. 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. is ipywidgets suppressing the errors? I seem to recall that all the errors were outputted to the console in the GUI. Is that not the case? 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. Maybe not all executions are logged to the console. Only script that is executed within a context manager of the logging window are logged. One of the first bugs I looked into was a plotting element that wasn't set within a context manager... |
||
# Handle the exception and print it | ||
print(f"An error occurred: {e}") | ||
return wrapper | ||
return _unlink_relink | ||
|
||
|
@@ -273,11 +278,26 @@ def _dynamic_rerender(fig): | |
fig.tight_layout() | ||
|
||
|
||
def _plot_on_axes(b, widgets_simulation, widgets_plot_type, | ||
target_simulations, | ||
spectrogram_colormap_selection, dipole_smooth, | ||
max_spectral_frequency, dipole_scaling, widgets, data, | ||
fig_idx, fig, ax, existing_plots): | ||
def create_plot_config(data, dipole_scaling, data_scaling, dipole_smooth, | ||
data_smooth, max_spectral_frequency, | ||
spectrogram_colormap_selection): | ||
|
||
gtdang marked this conversation as resolved.
Show resolved
Hide resolved
|
||
target_is_sim = data['net'] is not None | ||
scaling = dipole_scaling.value if target_is_sim else data_scaling.value | ||
smooth = dipole_smooth.value if target_is_sim else data_smooth.value | ||
return { | ||
"max_spectral_frequency": max_spectral_frequency.value, | ||
"dipole_scaling": scaling, | ||
"dipole_smooth": smooth, | ||
"spectrogram_cm": spectrogram_colormap_selection.value | ||
} | ||
|
||
|
||
def _plot_on_axes(b, simulations_widget, widgets_plot_type, | ||
gtdang marked this conversation as resolved.
Show resolved
Hide resolved
|
||
data_widget, | ||
spectrogram_colormap_selection, max_spectral_frequency, | ||
dipole_smooth, dipole_scaling, data_smooth, data_scaling, | ||
widgets, data, fig_idx, fig, ax, existing_plots): | ||
"""Plotting different types of data on the given axes. | ||
|
||
Now this function is also responsible for comparing multiple simulations, | ||
|
@@ -314,7 +334,7 @@ def _plot_on_axes(b, widgets_simulation, widgets_plot_type, | |
existing_plots : ipywidgets.VBox | ||
A VBox widget that contains all the existing plots. | ||
""" | ||
sim_name = widgets_simulation.value | ||
sim_name = simulations_widget.value | ||
plot_type = widgets_plot_type.value | ||
# disable add plots for types that do not support overlay | ||
if plot_type in _no_overlay_plot_types: | ||
|
@@ -324,33 +344,38 @@ def _plot_on_axes(b, widgets_simulation, widgets_plot_type, | |
widgets_plot_type.disabled = True | ||
|
||
single_simulation = data['simulations'][sim_name] | ||
|
||
plot_config = { | ||
"max_spectral_frequency": max_spectral_frequency.value, | ||
"dipole_scaling": dipole_scaling.value, | ||
"dipole_smooth": dipole_smooth.value, | ||
"spectrogram_cm": spectrogram_colormap_selection.value | ||
} | ||
simulation_plot_config = create_plot_config( | ||
data=single_simulation, | ||
dipole_scaling=dipole_scaling, | ||
data_scaling=data_scaling, | ||
dipole_smooth=dipole_smooth, | ||
data_smooth=data_smooth, | ||
max_spectral_frequency=max_spectral_frequency, | ||
spectrogram_colormap_selection=spectrogram_colormap_selection) | ||
|
||
dpls_processed = _update_ax(fig, ax, single_simulation, sim_name, | ||
plot_type, plot_config) | ||
plot_type, simulation_plot_config) | ||
|
||
# If target_simulations is not None and we are plotting a dipole, | ||
# we need to plot the target dipole as well. | ||
if target_simulations.value in data['simulations'].keys( | ||
if data_widget.value in data['simulations'].keys( | ||
) and plot_type == 'current dipole': | ||
|
||
target_sim_name = target_simulations.value | ||
target_sim_name = data_widget.value | ||
target_sim = data['simulations'][target_sim_name] | ||
|
||
# plot the target dipole. | ||
# disable scaling for the target dipole. | ||
plot_config['dipole_scaling'] = 1. | ||
data_plot_config = create_plot_config( | ||
data=target_sim, | ||
dipole_scaling=dipole_scaling, | ||
data_scaling=data_scaling, | ||
dipole_smooth=dipole_smooth, | ||
data_smooth=data_smooth, | ||
max_spectral_frequency=max_spectral_frequency, | ||
spectrogram_colormap_selection=spectrogram_colormap_selection) | ||
|
||
# plot the target dipole. | ||
target_dpl_processed = _update_ax( | ||
fig, ax, target_sim, target_sim_name, plot_type, | ||
plot_config)[0] # we assume there is only one dipole. | ||
data_plot_config)[0] # we assume there is only one dipole. | ||
|
||
# calculate the RMSE between the two dipoles. | ||
t0 = 0.0 | ||
|
@@ -401,14 +426,15 @@ def _get_ax_control(widgets, data, fig_idx, fig, ax): | |
layout = Layout(width="98%") | ||
simulation_names = tuple(data['simulations'].keys()) | ||
sim_name_default = simulation_names[-1] | ||
|
||
if len(simulation_names) == 0: | ||
simulation_names = [ | ||
"None", | ||
] | ||
|
||
simulation_selection = Dropdown( | ||
options=simulation_names, | ||
value=sim_name_default, | ||
value=simulation_names[0], | ||
description='Simulation Data:', | ||
disabled=False, | ||
layout=layout, | ||
|
@@ -431,8 +457,12 @@ def _get_ax_control(widgets, data, fig_idx, fig, ax): | |
style=analysis_style, | ||
) | ||
|
||
tagert_names = simulation_names[:-1] | ||
if len(simulation_names) > 1: | ||
tagert_names = simulation_names[1:] | ||
|
||
target_data_selection = Dropdown( | ||
options=simulation_names[:-1] + ('None',), | ||
options=tagert_names + ('None',), | ||
value='None', | ||
description='Data to Compare:', | ||
disabled=False, | ||
|
@@ -447,16 +477,33 @@ def _get_ax_control(widgets, data, fig_idx, fig, ax): | |
layout=layout, | ||
style=analysis_style, | ||
) | ||
dipole_smooth = FloatText(value=30, | ||
description='Dipole Smooth Window (ms):', | ||
disabled=False, | ||
layout=layout, | ||
style=analysis_style) | ||
dipole_scaling = FloatText(value=3000, | ||
description='Dipole Scaling:', | ||
disabled=False, | ||
layout=layout, | ||
style=analysis_style) | ||
simulation_dipole_smooth = FloatText( | ||
value=30, | ||
description='Dipole Smooth Window (ms):', | ||
disabled=False, | ||
layout=layout, | ||
style=analysis_style) | ||
|
||
simulation_dipole_scaling = FloatText( | ||
value=3000, | ||
description='Simulation Dipole Scaling:', | ||
disabled=False, | ||
layout=layout, | ||
style=analysis_style) | ||
|
||
data_dipole_smooth = FloatText( | ||
value=0, | ||
description='Data Smooth Window (ms):', | ||
disabled=False, | ||
layout=layout, | ||
style=analysis_style) | ||
|
||
data_dipole_scaling = FloatText( | ||
value=1, | ||
description='Data Dipole Scaling:', | ||
disabled=False, | ||
layout=layout, | ||
style=analysis_style) | ||
|
||
max_spectral_frequency = FloatText( | ||
value=100, | ||
|
@@ -501,13 +548,15 @@ def _on_plot_type_change(new_plot_type): | |
plot_button.on_click( | ||
partial( | ||
_plot_on_axes, | ||
widgets_simulation=simulation_selection, | ||
simulations_widget=simulation_selection, | ||
widgets_plot_type=plot_type_selection, | ||
target_simulations=target_data_selection, | ||
data_widget=target_data_selection, | ||
spectrogram_colormap_selection=spectrogram_colormap_selection, | ||
dipole_smooth=dipole_smooth, | ||
max_spectral_frequency=max_spectral_frequency, | ||
dipole_scaling=dipole_scaling, | ||
dipole_smooth=simulation_dipole_smooth, | ||
dipole_scaling=simulation_dipole_scaling, | ||
data_smooth=data_dipole_smooth, | ||
data_scaling=data_dipole_scaling, | ||
widgets=widgets, | ||
data=data, | ||
fig_idx=fig_idx, | ||
|
@@ -517,8 +566,9 @@ def _on_plot_type_change(new_plot_type): | |
)) | ||
|
||
vbox = VBox([ | ||
simulation_selection, plot_type_selection, target_data_selection, | ||
dipole_smooth, dipole_scaling, max_spectral_frequency, | ||
plot_type_selection, simulation_selection, simulation_dipole_smooth, | ||
simulation_dipole_scaling, target_data_selection, data_dipole_smooth, | ||
data_dipole_scaling, max_spectral_frequency, | ||
spectrogram_colormap_selection, | ||
HBox( | ||
[plot_button, clear_button], | ||
|
@@ -816,11 +866,11 @@ def _simulate_edit_figure(self, fig_name, ax_name, simulation_name, | |
ax_control_tabs.selected_index = ax_idx | ||
|
||
# ax config | ||
simulation_ctrl = ax_control_tabs.children[ax_idx].children[0] | ||
simulation_ctrl = ax_control_tabs.children[ax_idx].children[1] | ||
# return simulation_ctrl | ||
simulation_ctrl.value = simulation_name | ||
|
||
plot_type_ctrl = ax_control_tabs.children[ax_idx].children[1] | ||
plot_type_ctrl = ax_control_tabs.children[ax_idx].children[0] | ||
plot_type_ctrl.value = plot_type | ||
|
||
config_name_idx = { | ||
|
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.
why do you need try-except? it's not a good idiom in Python ... replacing with
if
-else
is preferable when possibleThere 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.
From Zen of Python:
see also: https://peps.python.org/pep-0008/#programming-recommendations
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.
There is internal code in the
unlink
call doesnt check the state of some attributes, causingout of bound
s exceptions that are unhandled by hnn or the traitlets package. Additionally, thef
handler call also throws exceptions unhandled by hnn. I can explicitly refer this exceptions in thetry-except
block or useif-else
, but what if more exceptions come out? I am open to suggestions. Also, I can jut remove this block and create an issue for a future PR.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.
Camilo and I discussed this. It is useful as we are actively developing new features for the GUI over the next few months. Often times we're not debugging through a live GUI. We're going to keep it commented in pushes but will remove it entirely once the GUI is in production.
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.
If you need it for debugging, it's fine. I would rather have it uncommented than not. But with an explanation why it's needed in the comments. I also realized that we're missing explanation for the unlink/relink logic in the code itself. I had to dig through github issues to understand why that exists. We should have a two-line comment with a pointer to an open github issue (on ipywidgets) suggesting when this logic can be removed. I worry we're adding patch on top of patch.
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.
#742
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.
The try-except makes me nervous. It will suppress genuine errors. Please remove it and address what's necessary in a separate PR ... one never finds the time to go back and fix things.
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.
I'm confused. This is actually going to print all errors and exceptions and not suppress any errors.
You could add a
raise e
if you want the program to stop upon finding an error. Though that's not ideal in the GUI as you will need to restart the GUI.In production code you typically don't want to use
except Exception
, but something more specific likeexcept ValueError
, but that's only once you know the common exceptions that occur (and need to handle them in a specific way). The generalException
is good during development to find out what those common errors are.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.
Anyway, this logic is pretty minor and quick to add back while debugging so we should just remove it and not discuss it any further. But I did create issue #737 this week to remind CCV to review our work and remove any dev-related helper clauses/function before release that may slip through PRs.
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.
Thanks :) The print statement isn't the same level as an error but it's certainly better than nothing. I would have preferred to have the errors thrown using the ipywidgets logging infrastructure as everything would then be visible in the same consolidated output panel. We can discuss more once I'm back from vacation. In the meantime, if you can inject the try-except as you need it, that would be great!