search

jonescompneurolab / hnn-core

Simulation and optimization of neural circuits for MEG/EEG source estimates
https://jonescompneurolab.github.io/hnn-core/
BSD 3-Clause "New" or "Revised" License
55 stars 52 forks source link

GUI: Add warning for GUI spectrogram if time too short #937

Open asoplata opened 1 week ago

asoplata commented 1 week ago

It seems that the hotfix I made to improve the spectrogram visuals for the Alpha/Beta tutorial here https://github.com/jonescompneurolab/hnn-core/pull/928 broke spectrogram display in the GUI when running the default simulation, at least for me. Before that commit it works, but after that commit, if I try to make a Dipole-Spectrogram figure after having run the default simulation that is loaded upon GUI start, I get this:

image

This is almost certainly related to the length of time needed for analysis generation, which is why I needed to lengthen the time for the tests to pass here: https://github.com/jonescompneurolab/hnn-core/pull/928/commits/c0779ae733036308c541370d0932862bae48528f . How this apparently passed pytest runs, especially test_viz and test_gui, I have no idea.

The fast solution is simple: as we have done recently with the smoothing factor, we introduce a new hotfix to revert the Morlet cycle frequency divisor back to 8. Independent of the default, we should also add a widget so that users can customize the divisor itself.

jasmainak commented 1 week ago

I think we have to accept that the GUI is meant for exploration and decide what features make most sense for teaching/exploratory purpose. Then export the data and continue analysis in HNN-core ... we'll soon have combinatorial problems to debug the more features we add to the GUI

asoplata commented 1 week ago

That is a good point that we need to keep GUI additions to the minimum. We discussed this in the Dev meeting on 2024-11-15, and @ntolley and @dylansdaniels confirmed that the above is expected behavior.

In the short term, I will make a PR for this issue that adds a warning to the user. The warning will print to the main log in the event that the spectrogram is not generated due to the length of simulation being too low for the provided frequencies. This is why the spectrogram is not being shown in the default simulation of the GUI - they have clarified that the default GUI simulation is based off of the simulation used in the ERP tutorial, for which it is not useful to calculate a spectrogram.

In the meantime (for Milestone v0.4), @ntolley is going to work on changing plot_tfr_morlet so that it automatically calculates its time x frequency analysis to be appropriate to the activity.