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.

Sign up for GitHub

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

Jump to bottom

[MRG] Fix dipole plot scale and smooth factors #730

Merged
merged 13 commits into from
Mar 29, 2024
Merged

[MRG] Fix dipole plot scale and smooth factors #730

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
5f235be
reorganizing visualization tab widgets. adding two additional float f…
kmilo9999 Mar 15, 2024
95ed9c1
fixing gui test and funciton args name
kmilo9999 Mar 18, 2024
3ac1386
fix linting errors
kmilo9999 Mar 18, 2024
ccb255a
fix lint errors in gui.py
kmilo9999 Mar 18, 2024
d480947
fixing gui tests
kmilo9999 Mar 18, 2024
affbfda
adding scaling and smooth input parameters for data comparison visual…
kmilo9999 Mar 21, 2024
e47a1e8
applied code review suggestions. Added functio to avoid repeated code
kmilo9999 Mar 22, 2024
e3ae1c4
adding docustring to new funtion. removing unused is_loaded_data
kmilo9999 Mar 22, 2024
c4f2319
fixing flake8 erros
kmilo9999 Mar 23, 2024
3be7a2d
removed logic to determine if data is a sim or loaded data
kmilo9999 Mar 26, 2024
7de6d8f
MAINT: commenting out try-except block
kmilo9999 Mar 26, 2024
2dcf972
MAINT: uncommenting atry-except dev code in viz_manager
kmilo9999 Mar 27, 2024
e83b008
MAINT: remove try-except block
kmilo9999 Mar 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 83 additions & 41 deletions hnn_core/gui/_viz_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [
Expand Down Expand Up @@ -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:
Copy link
Collaborator

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 possible

Copy link
Collaborator

Choose a reason for hiding this comment

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

From Zen of Python:

Errors should never pass silently.
Unless explicitly silenced.

see also: https://peps.python.org/pep-0008/#programming-recommendations

Copy link
Collaborator Author

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, causing out of bounds exceptions that are unhandled by hnn or the traitlets package. Additionally, the f handler call also throws exceptions unhandled by hnn. I can explicitly refer this exceptions in the try-except block or use if-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.

Copy link
Collaborator

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.

Copy link
Collaborator

@jasmainak jasmainak Mar 27, 2024 •
edited
Loading

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#742

Copy link
Collaborator

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.

Copy link
Collaborator

@gtdang gtdang Mar 29, 2024 •
edited
Loading

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.

try:
    function()
except Exception as e:
    print(f"{e}")

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 like except ValueError, but that's only once you know the common exceptions that occur (and need to handle them in a specific way). The general Exception is good during development to find out what those common errors are.

Copy link
Collaborator

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.

Copy link
Collaborator

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!

# 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:
Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.
This was my way to recognize the exception's message. Should I ditch these lines or swap them for something a bit more polished?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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

Expand Down Expand Up @@ -273,10 +278,11 @@ def _dynamic_rerender(fig):
fig.tight_layout()


def _plot_on_axes(b, widgets_simulation, widgets_plot_type,
target_simulations,
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, dipole_smooth,
max_spectral_frequency, dipole_scaling, widgets, data,
max_spectral_frequency, dipole_scaling, data_smooth,
data_scaling, widgets, data,
gtdang marked this conversation as resolved.
Show resolved Hide resolved
fig_idx, fig, ax, existing_plots):
"""Plotting different types of data on the given axes.

Expand Down Expand Up @@ -314,7 +320,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:
Expand All @@ -324,33 +330,44 @@ def _plot_on_axes(b, widgets_simulation, widgets_plot_type,
widgets_plot_type.disabled = True

single_simulation = data['simulations'][sim_name]

plot_config = {
target_is_sim = True if single_simulation['net'] is not None else False
scaling = dipole_scaling.value if target_is_sim else data_scaling.value
smooth = dipole_smooth.value if target_is_sim else data_smooth.value
simulation_plot_config = {
"max_spectral_frequency": max_spectral_frequency.value,
"dipole_scaling": dipole_scaling.value,
"dipole_smooth": dipole_smooth.value,
"dipole_scaling": scaling,
"dipole_smooth": smooth,
"spectrogram_cm": spectrogram_colormap_selection.value
}

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]
target_is_sim = True if target_sim['net'] is not None else False
scaling = dipole_scaling.value if target_is_sim else data_scaling.value
smooth = dipole_smooth.value if target_is_sim else data_smooth.value
data_plot_config = {
"max_spectral_frequency": max_spectral_frequency.value,
"dipole_scaling": scaling,
"dipole_smooth": smooth,
"spectrogram_cm": spectrogram_colormap_selection.value
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like repeat logic here. Perhaps create a function for compiling the data_plot_config dict based on if the data is a simulation or not?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I created a new function with the repeated code. Le me know your thoughts. I think it could improve


# plot the target dipole.
# disable scaling for the target dipole.
plot_config['dipole_scaling'] = 1.
simulation_plot_config['dipole_scaling'] = 1.
gtdang marked this conversation as resolved.
Show resolved Hide resolved

# 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
Expand Down Expand Up @@ -401,14 +418,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,
Expand All @@ -431,8 +449,12 @@ def _get_ax_control(widgets, data, fig_idx, fig, ax):
style=analysis_style,
)

tagert_names = simulation_names[0:-1]
gtdang marked this conversation as resolved.
Show resolved Hide resolved
if len(simulation_names) > 1:
tagert_names = simulation_names[1:]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure I'm following here. It gets rid of the first simulation there is more than one set of data? Why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is a bug at creating a new plot using the "Make figure" button. The "data to compare" dropdown doesnt show all the available datasets to compare. They only show up when the value of the "Simulation Data" dropdown changes (a callback function is invoked and updates both lists). These lines initialize the list of available options without having to change the dropdowns values.


target_data_selection = Dropdown(
options=simulation_names[:-1] + ('None',),
options=tagert_names + ('None',),
value='None',
description='Data to Compare:',
disabled=False,
Expand All @@ -447,16 +469,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,
Expand Down Expand Up @@ -501,13 +540,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,
dipole_smooth=simulation_dipole_smooth,
gtdang marked this conversation as resolved.
Show resolved Hide resolved
max_spectral_frequency=max_spectral_frequency,
dipole_scaling=dipole_scaling,
dipole_scaling=simulation_dipole_scaling,
data_smooth=data_dipole_smooth,
data_scaling=data_dipole_scaling,
widgets=widgets,
data=data,
fig_idx=fig_idx,
Expand All @@ -517,8 +558,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],
Expand Down Expand Up @@ -816,11 +858,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 = {
Expand Down
6 changes: 6 additions & 0 deletions hnn_core/gui/gui.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1124,6 +1124,12 @@ def load_drive_and_connectivity(params, log_out, drives_out,
layout)


def is_loaded_data(simulation_data):
if 'net' in simulation_data:
return True
return False


kmilo9999 marked this conversation as resolved.
Show resolved Hide resolved
def on_upload_data_change(change, data, viz_manager, log_out):
if len(change['owner'].value) == 0:
logger.info("Empty change")
Expand Down
4 changes: 2 additions & 2 deletions hnn_core/tests/test_gui.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ def test_gui_edit_figure():
assert len(axes_config_tabs.children) == n_figs

axes_config = axes_config_tabs.children[-1].children[1]
simulation_selection = axes_config.children[0].children[0]
simulation_selection = axes_config.children[0].children[1]
assert simulation_selection.options == tuple(sim_names[:n_figs])
plt.close('all')

Expand All @@ -378,7 +378,7 @@ def test_gui_figure_overlay():
for controls in tab.children[1].children:
add_plot_button = controls.children[-2].children[0]
clear_ax_button = controls.children[-2].children[1]
plot_type_selection = controls.children[1]
plot_type_selection = controls.children[0]

assert plot_type_selection.disabled is True
clear_ax_button.click()
Expand Down
Loading