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.

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

Fix length error in sonify.time_frequency #374

Merged
merged 4 commits into from
Mar 26, 2024
Merged

Conversation

bmcfee
Copy link
Collaborator

@bmcfee bmcfee commented Mar 25, 2024

This PR fixes #371 by separating out the slicing logic for time_frequency generation.

I've unskipped the test that was previously failing and things seem good to go.

@bmcfee bmcfee added the bug label Mar 25, 2024
@bmcfee bmcfee added this to the 0.8 milestone Mar 25, 2024
@bmcfee bmcfee changed the title fixed #371 Fix length error in sonify.time_frequency Mar 25, 2024
@codecov-commenter
Copy link

codecov-commenter commented Mar 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.32%. Comparing base (7997fdf) to head (76d9500).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #374      +/-   ##
==========================================
+ Coverage   88.31%   88.32%   +0.01%     
==========================================
  Files          19       19              
  Lines        2875     2878       +3     
==========================================
+ Hits         2539     2542       +3     
  Misses        336      336              
Flag Coverage Δ
unittests 88.32% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Contributor

@leighsmith leighsmith left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@bmcfee
Copy link
Collaborator Author

bmcfee commented Mar 26, 2024

Test failure is unrelated, appears to be a resurgence of a heisenbug in bsseval on osx. 🤦

@bmcfee
Copy link
Collaborator Author

bmcfee commented Mar 26, 2024

I added one more test here to cover the case where the input length is shorter than the provided input.

Otherwise I think this is good to go. We shouldn't let the OSX failure block merging this PR.

@bmcfee
Copy link
Collaborator Author

bmcfee commented Mar 26, 2024

Lol, rerunning the tests passed this time with no changes. OSX is great.

@bmcfee bmcfee merged commit e299244 into mir-evaluation:main Mar 26, 2024
8 checks passed
@bmcfee bmcfee deleted the fix-371 branch March 26, 2024 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sonify.time_frequency regression
3 participants