Skip to content

fix: plotting in colab#317

Merged
fabiocat93 merged 23 commits into
mainfrom
fix/plotting
Aug 14, 2025
Merged

fix: plotting in colab#317
fabiocat93 merged 23 commits into
mainfrom
fix/plotting

Conversation

@satra

@satra satra commented Apr 20, 2025

Copy link
Copy Markdown
Collaborator

Description

The colab notebook wasn't behaving as expected. This updates some of the plotting routines and updates the notebook for both plotting fix and an import error.

Related Issue(s)

closes #315

Motivation and Context

would be nice to see waves and spectrograms on colab.

How Has This Been Tested?

Manually on colab

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • My code follows the code style of this project.

@satra satra reopened this Apr 20, 2025
@satra
satra marked this pull request as ready for review April 20, 2025 19:48
@codecov-commenter

codecov-commenter commented Apr 21, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.57895% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.81%. Comparing base (113721a) to head (dabcd86).
⚠️ Report is 1091 commits behind head on main.

Files with missing lines Patch % Lines
src/senselab/utils/tasks/plotting.py 0.00% 8 Missing ⚠️
src/senselab/audio/tasks/plotting/plotting.py 76.00% 6 Missing ⚠️
src/tests/audio/tasks/plotting_test.py 98.72% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #317      +/-   ##
==========================================
+ Coverage   60.24%   65.81%   +5.57%     
==========================================
  Files         113      140      +27     
  Lines        4017     6196    +2179     
==========================================
+ Hits         2420     4078    +1658     
- Misses       1597     2118     +521     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@fabiocat93

Copy link
Copy Markdown
Collaborator

Nice catch in the related issue. Is this PR still a WIP? It doesn't seem to fix the issue on my end

@satra

satra commented Apr 21, 2025

Copy link
Copy Markdown
Collaborator Author

to test this, you'll have to point it to the right senselab install in the notebook. by default it points to pypi senselab.

! pip install 'senselab[audio] @ git+https://github.com/sensein/senselab.git@refs/pull/317/head'

@fabiocat93

Copy link
Copy Markdown
Collaborator

to test this, you'll have to point it to the right senselab install in the notebook. by default it points to pypi senselab.

! pip install 'senselab[audio] @ git+https://github.com/sensein/senselab.git@refs/pull/317/head'

thank you. I tested it both locally and on G Colab and still don't see the plots when I try to run your minimal demo in the issue, which makes me think that the problem is not entirely solved yet. If we speak about the 00_getting_started.ipynb tutorial, I see the plots already with the solution in the main branch

@satra

satra commented Apr 21, 2025 via email

Copy link
Copy Markdown
Collaborator Author

@fabiocat93

Copy link
Copy Markdown
Collaborator

the minimal demo will not work because the solution requires redoing matplotlib inline/ipympl after senselab is imported. If you do that in the minimal demo the plots will work. See the corresponding update in the PR notebook.

On Mon, Apr 21, 2025, 1:15 PM Fabio Catania @.> wrote: to test this, you'll have to point it to the right senselab install in the notebook. by default it points to pypi senselab. ! pip install 'senselab[audio] @ @./pull/317/head' thank you. I tested it both locally and on G Colab and still don't see the plots when I try to run your minimal demo in the issue, which makes me think that the problem is not entirely solved yet. If we speak about the 00_getting_started.ipynb tutorial, I see the plots already with the solution in the main branch — Reply to this email directly, view it on GitHub <#317 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABM57Y2USAOXDTL6AQO55L22URUVAVCNFSM6AAAAAB3PWBLUSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQMJZGA2TCMZTGI . You are receiving this because you modified the open/close state.Message ID: @.> fabiocat93 left a comment (sensein/senselab#317) <#317 (comment)> to test this, you'll have to point it to the right senselab install in the notebook. by default it points to pypi senselab. ! pip install 'senselab[audio] @ @./pull/317/head' thank you. I tested it both locally and on G Colab and still don't see the plots when I try to run your minimal demo in the issue, which makes me think that the problem is not entirely solved yet. If we speak about the 00_getting_started.ipynb tutorial, I see the plots already with the solution in the main branch — Reply to this email directly, view it on GitHub <#317 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABM57Y2USAOXDTL6AQO55L22URUVAVCNFSM6AAAAAB3PWBLUSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQMJZGA2TCMZTGI . You are receiving this because you modified the open/close state.Message ID: @.***>

It doesn't seem to be the case
Screenshot 2025-04-21 at 1 25 15 PM

@satra

satra commented Apr 21, 2025

Copy link
Copy Markdown
Collaborator Author

see this. technically the plotting issue can be obviated without any change in code (just in the notebook).

image

the changes in this PR make plotting more accessible programmatically. i didn't do any plotting enhancements, but there a few we should do like:

  • joint wave + spectrogram plots (single row, multi-row - as we had originally implemented in b2aiprep)
  • allow passing efficiency parameters to routines (reducing points, columns)
  • add support for screen resolution based plotting (like seaborn)

@ibevers

ibevers commented Apr 30, 2025

Copy link
Copy Markdown
Collaborator

CI not passing may be caused by a CI configuration issue--see here: #321

@ibevers ibevers left a comment

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.

This looks great. I see that there are no unit tests for the two plotting.py modules. Would it be helpful for me to add unit tests for these @satra?

@satra

satra commented Jun 11, 2025

Copy link
Copy Markdown
Collaborator Author

@ibevers - i think it's always useful to add tests. so please take this over.

@ibevers

ibevers commented Jun 11, 2025

Copy link
Copy Markdown
Collaborator

@ibevers - i think it's always useful to add tests. so please take this over.

@satra okay!

@ibevers ibevers self-assigned this Jun 11, 2025
Comment thread pyproject.toml Outdated
@ibevers ibevers added the minor Minor release label Aug 13, 2025
@ibevers
ibevers self-requested a review August 14, 2025 00:40

@ibevers ibevers left a comment

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.

Seems mergable to me.

@ibevers

ibevers commented Aug 14, 2025

Copy link
Copy Markdown
Collaborator

@fabiocat93 mergable?

@fabiocat93 fabiocat93 removed the minor Minor release label Aug 14, 2025
@fabiocat93

Copy link
Copy Markdown
Collaborator

there a few we should do like:

  • joint wave + spectrogram plots (single row, multi-row - as we had originally implemented in b2aiprep)
  • allow passing efficiency parameters to routines (reducing points, columns)
  • add support for screen resolution based plotting (like seaborn)

@satra @ibevers these aspects haven't been solved in this PR and I opened an issue for those: #361
@ibevers idk how experienced you are with the b2aiprep utilities for plotting, but if you think it's easy to integate some of these aspects from there to senselab, feel free to take over (no pressure)

@satra

satra commented Aug 14, 2025

Copy link
Copy Markdown
Collaborator Author

@fabiocat93 - that seems separate from this issue and perhaps should be handled in a separate PR. if this is ready, feel free to merge this.

@fabiocat93

Copy link
Copy Markdown
Collaborator

@fabiocat93 - that seems separate from this issue and perhaps should be handled in a separate PR. if this is ready, feel free to merge this.

yes, indeed that's what I am saying (which is why I created a separate issue).

Coming back to the matter of this PR, unfortunately,

  • I tested it on Colab and works fine
  • I tested it locally and keep having the same visualization problem.

This doens't work:

from senselab.audio.tasks.plotting.plotting import plot_waveform
plot_waveform(audio1);

This is a workaround to make see the plot:

from senselab.audio.tasks.plotting.plotting import plot_waveform
%matplotlib inline

plot_waveform(audio1);

@satra

satra commented Aug 14, 2025

Copy link
Copy Markdown
Collaborator Author

did you see this comment: #317 (comment)

@satra

satra commented Aug 14, 2025

Copy link
Copy Markdown
Collaborator Author

the changes in this PR are unrelated to the original issue except for part of the notebook change which implements that comment.

@fabiocat93

Copy link
Copy Markdown
Collaborator

the changes in this PR are unrelated to the original issue except for part of the notebook change which implements that comment.

I have seen your comment. And I know that including %matplotlib inline is a classic way to run matplotlib in a notebook.
My point is that the notebook changes in this PR (i.e., implementing the %matplotlib inline strategy) don't solve the issue locally (the Figure didn't show up before and doesn't show up now)

Screenshot 2025-08-14 at 11 37 38 AM

It seems that %matplotlib inline is required right before the line: plot_waveform(audio);

@satra

satra commented Aug 14, 2025

Copy link
Copy Markdown
Collaborator Author

it has to be done after the first time senselab is imported, hence the change in the notebook.

@fabiocat93

Copy link
Copy Markdown
Collaborator

it has to be done after the first time senselab is imported, hence the change in the notebook.

which wasn't the case outside of G Colab. Fixed it, also in the rest of the relevant notebooks in the tutorials/ folder. Thanks everyone

@fabiocat93
fabiocat93 merged commit 4aadfb6 into main Aug 14, 2025
19 of 23 checks passed
@fabiocat93
fabiocat93 deleted the fix/plotting branch October 28, 2025 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: tutorial waveform plot does not show in google colab

4 participants