-
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
[MRG] Fix dipole plot scale and smooth factors #730
Conversation
hnn_core/gui/_viz_manager.py
Outdated
|
||
return result | ||
return result | ||
except Exception as e: |
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.
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 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?
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 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 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?
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.
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...
@ntolley @dylansdaniels What is the real purpose of the default smooth and scaling to 30 and 3000 respectively? What do you think about setting the loaded data default values to smooth = 0 and scaling = 1 ? |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #730 +/- ##
==========================================
+ Coverage 92.66% 92.73% +0.06%
==========================================
Files 27 27
Lines 4911 4917 +6
==========================================
+ Hits 4551 4560 +9
+ Misses 360 357 -3 ☔ View full report in Codecov by Sentry. |
…ields for data comparison
d03a7ca
to
affbfda
Compare
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 Camilo! Left some comments.
hnn_core/gui/_viz_manager.py
Outdated
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 | ||
} |
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.
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?
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 created a new function with the repeated code. Le me know your thoughts. I think it could improve
hnn_core/gui/_viz_manager.py
Outdated
tagert_names = simulation_names[0:-1] | ||
if len(simulation_names) > 1: | ||
tagert_names = simulation_names[1:] |
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.
Not sure I'm following here. It gets rid of the first simulation there is more than one set of data? Why?
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 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.
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.
Looking good Camilo. Just some minor edits.
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.
Looks great! Should be ready to merge once all checks pass!
hnn_core/gui/_viz_manager.py
Outdated
# Unlink the widgets using the provided link object | ||
link_attribute: link = getattr(self, attribute) | ||
link_attribute.unlink() | ||
try: |
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 possible
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.
From Zen of Python:
Errors should never pass silently.
Unless explicitly silenced.
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, causing out of bound
s 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.
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.
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.
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.
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!
Thanks Camilo! |
* reorganizing visualization tab widgets. adding two additional float fields for data comparison * fixing gui test and funciton args name * fix linting errors * fix lint errors in gui.py * fixing gui tests * adding scaling and smooth input parameters for data comparison visualization * applied code review suggestions. Added functio to avoid repeated code * adding docustring to new funtion. removing unused is_loaded_data * fixing flake8 erros * removed logic to determine if data is a sim or loaded data * MAINT: commenting out try-except block * MAINT: uncommenting atry-except dev code in viz_manager * MAINT: remove try-except block
* reorganizing visualization tab widgets. adding two additional float fields for data comparison * fixing gui test and funciton args name * fix linting errors * fix lint errors in gui.py * fixing gui tests * adding scaling and smooth input parameters for data comparison visualization * applied code review suggestions. Added functio to avoid repeated code * adding docustring to new funtion. removing unused is_loaded_data * fixing flake8 erros * removed logic to determine if data is a sim or loaded data * MAINT: commenting out try-except block * MAINT: uncommenting atry-except dev code in viz_manager * MAINT: remove try-except block
* reorganizing visualization tab widgets. adding two additional float fields for data comparison * fixing gui test and funciton args name * fix linting errors * fix lint errors in gui.py * fixing gui tests * adding scaling and smooth input parameters for data comparison visualization * applied code review suggestions. Added functio to avoid repeated code * adding docustring to new funtion. removing unused is_loaded_data * fixing flake8 erros * removed logic to determine if data is a sim or loaded data * MAINT: commenting out try-except block * MAINT: uncommenting atry-except dev code in viz_manager * MAINT: remove try-except block
parent 289b85b author samadpls <[email protected]> 1710875966 +0500 committer samadpls <[email protected]> 1712146472 +0500 # This is a combination of 13 commits. parent 289b85b author samadpls <[email protected]> 1710875966 +0500 committer samadpls <[email protected]> 1712146405 +0500 # This is a combination of 4 commits.tree de20b388c64cca5a81e713c9270d6c1a3af12f93 parent 289b85b author samadpls <[email protected]> 1710875966 +0500 committer samadpls <[email protected]> 1712146383 +0500 # This is a combination of 3 commits. # This is the 1st commit message: Added `kwargs` options to `plot_spikes_hist` Signed-off-by: samadpls <[email protected]> Updated `whats_new.rst` with changes to plot_spikes_hist options using kwargs. Signed-off-by: samadpls <[email protected]> refactored Co-authored-by: Mainak Jas <[email protected]> refactored `kwargs_hist` Signed-off-by: samadpls <[email protected]> Updated `test_cell_response` with `kwargs_hist` Signed-off-by: samadpls <[email protected]> Update doc/whats_new.rst Co-authored-by: Mainak Jas <[email protected]> removed linewidth param Signed-off-by: GitHub <[email protected]> Added assertion Signed-off-by: samadpls <[email protected]> Added `kwargs` options to `plot_spikes_hist` Signed-off-by: samadpls <[email protected]> # This is the commit message #2: refactored Co-authored-by: Mainak Jas <[email protected]> # This is the commit message #3: refactored `kwargs_hist` Signed-off-by: samadpls <[email protected]> # This is the commit message jonescompneurolab#4: Update doc/whats_new.rst Co-authored-by: Mainak Jas <[email protected]> # This is the commit message jonescompneurolab#7: [MRG] Fix dipole plot scale and smooth factors (jonescompneurolab#730) * reorganizing visualization tab widgets. adding two additional float fields for data comparison * fixing gui test and funciton args name * fix linting errors * fix lint errors in gui.py * fixing gui tests * adding scaling and smooth input parameters for data comparison visualization * applied code review suggestions. Added functio to avoid repeated code * adding docustring to new funtion. removing unused is_loaded_data * fixing flake8 erros * removed logic to determine if data is a sim or loaded data * MAINT: commenting out try-except block * MAINT: uncommenting atry-except dev code in viz_manager * MAINT: remove try-except block # This is the commit message jonescompneurolab#8: [MRG] Added Virtual Environment Directories to `.gitignore` (jonescompneurolab#740) * Added virtual env * Updated * Refactored # This is the commit message jonescompneurolab#9: fix: correct pick_connection error where connections not relevant to the search criteria were returned when a search for non-existent connections was preformed # This is the commit message #10: style: clarify comments # This is the commit message jonescompneurolab#11: tests: added test for searching for a cell connection that is not configured in the network. # This is the commit message jonescompneurolab#12: style: flake8 edits # This is the commit message #13: refactor: convert to lists # This is the commit message jonescompneurolab#15: chore: Corrected whats_new.rst tense. # This is the commit message jonescompneurolab#17: refactor: simplified logic # This is the commit message #18: reorganizing visualization tab widgets. adding two additional float fields for data comparison fix linting errors fix lint errors in gui.py adding scaling and smooth input parameters for data comparison visualization applied code review suggestions. Added functio to avoid repeated code adding docustring to new funtion. removing unused is_loaded_data MAINT: commenting out try-except block MAINT: uncommenting atry-except dev code in viz_manager
solves #717