Skip to content

[WIP] GUI: average spectrogram across trials instead of using first dipole#1215

Closed
Tusharjamdade wants to merge 5 commits intojonescompneurolab:masterfrom
Tusharjamdade:average-spectrogram-multi-trial
Closed

[WIP] GUI: average spectrogram across trials instead of using first dipole#1215
Tusharjamdade wants to merge 5 commits intojonescompneurolab:masterfrom
Tusharjamdade:average-spectrogram-multi-trial

Conversation

@Tusharjamdade
Copy link
Copy Markdown
Contributor

Fixes #1166

This PR fixes the spectrogram behavior in the GUI when a simulation contains multiple trials (n_trials > 1). Previously, only the first Dipole instance was used to compute the spectrogram, which did not accurately represent multi-trial simulations.

@Tusharjamdade
Copy link
Copy Markdown
Contributor Author

Hi @asoplata and @ntolley,
When you have a moment, could you please take a look at this? Thank you!

@asoplata
Copy link
Copy Markdown
Collaborator

asoplata commented Feb 3, 2026

Thanks for your PR Tushar, we will try to get to this PR (and your others) when we can; things are particularly busy right now. We appreciate your patience.

@Tusharjamdade
Copy link
Copy Markdown
Contributor Author

Hi maintainers, this PR is for GSoC 2026. Could you please add the gsoc-2026 label? I would also appreciate a review whenever you have time. Thank you!

@asoplata asoplata added the gsoc-2026 Use this label for "test" PRs where you are intending to demonstrate aptitude. label Mar 17, 2026
Comment thread hnn_core/gui/_viz_manager.py Outdated
ax=ax,
colorbar_inside=True,
show=False,
for dpl in dpls_copied:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hey @Tusharjamdade this is a great solution!

I wanted to draw your attention to the plot_tfr_morlet() function. As you have seen, there is indeed a method attached to the Dipole class that calls this function. However, if you import the function directly from viz.py, you can see that it accept a list of dipoles, and computes the average TFR similar to what you have done here

def plot_tfr_morlet(

Ultimately the line change will be much more subtle and look like

plot_tfr_morlet(dpls_copied, ...)

@Tusharjamdade Tusharjamdade force-pushed the average-spectrogram-multi-trial branch from 1f66984 to 12d4ee8 Compare March 19, 2026 19:48
@Tusharjamdade
Copy link
Copy Markdown
Contributor Author

Hi @ntolley,

Thank you for the review and for pointing me to the existing plot_tfr_morlet() function.
I’ve updated the implementation to use this utility directly with the list of dipoles instead of manually computing and averaging the TFR.

Please let me know if any further changes or refinements are needed.

@asoplata
Copy link
Copy Markdown
Collaborator

Hello Tushar,

Can you please write a test for your changes in test_gui.py? Specifically, all that is necessary would be a test that tests:

  1. For two GUI-run simulations that only use a single trial, the data in the spectrogram plot should be the same (I think this is the case but I'm not absolutely sure).
  2. For two GUI-run simulations where one simulation uses a single trial but the other uses multiple trials, the data in the spectrogram plot should be different.

Be warned that the code for HNNGUI and VizManager can be difficult to follow. I would recommend not trying to understand the classes in their entirety, but instead only focusing on the code that is necessary for these tests. It's fine if you are unable to successfully build the test.

@Tusharjamdade Tusharjamdade changed the title GUI: average spectrogram across trials instead of using first dipole [WIP] GUI: average spectrogram across trials instead of using first dipole Mar 24, 2026
@Tusharjamdade
Copy link
Copy Markdown
Contributor Author

@asoplata, thank you for suggesting the test for this functionality. I have implemented it as per your suggestions. It also helped me gain a better understanding of the HNNGUI and VizManager. Please let me know if any improvements are needed.

@asoplata
Copy link
Copy Markdown
Collaborator

Hello Tushar,

In the interest of merging work by other GSOC candidates, we've made the decision that we will not be merging this PR into master. To explain, there have been many Github Issues with multiple duplicate PRs by different GSOC candidates, and we can only merge one for each Issue. As explained in #1202, we've been trying to use these duplicate PRs as "exams" to vet candidates. Unfortunately, this PR solves one of the issues that has duplicates. Since you have already done an excellent job in this and other PRs, and even gotten two merged so far (nice!), we will move forward with merging a PR from a different GSOC candidate that solves this same issue. I apologize if you were not aware of this, since I should have made this more explicitly clear to all incoming GSOC candidate PRs initially.

Nonetheless, we will take your demonstration of technical and communication abilities in this and other PRs into account during our GSOC review process :) !

@asoplata asoplata closed this Mar 25, 2026
@github-project-automation github-project-automation Bot moved this from Backlog to Done in HNN Workspace Turbo 9000 Mar 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gsoc-2026 Use this label for "test" PRs where you are intending to demonstrate aptitude.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

GUI: For n_trials>1, spectrogram should show average of all spectrograms (in freq domain)

3 participants