Skip to content

Fix types in beat tracking code#1977

Merged
bmcfee merged 1 commit intolibrosa:1.0.0devfrom
emilazy:push-snotmtosmwqo
Sep 9, 2025
Merged

Fix types in beat tracking code#1977
bmcfee merged 1 commit intolibrosa:1.0.0devfrom
emilazy:push-snotmtosmwqo

Conversation

@emilazy
Copy link

@emilazy emilazy commented Aug 27, 2025

Reference Issue

What does this implement/fix? Explain your changes.

The development version of Numba 0.62.0 is unhappy about range being applied to floating point values here.

Any other comments?

@bmcfee
Copy link
Member

bmcfee commented Aug 28, 2025

Thanks for this - it appears to be the result of the fix for numba/numba#9909

I'd like to hold off on doing anything with this until 0.62 is final, or we somehow get #1528 off the ground, as we can't otherwise verify that it works properly in CI.

I think the solution implemented here looks fine, though the explicit cast to int for frames_per_beat may be premature since it gets cast back to float in the loop line that triggered the problem here.

@codecov
Copy link

codecov bot commented Aug 28, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.72%. Comparing base (e403272) to head (902bcca).
⚠️ Report is 54 commits behind head on 1.0.0dev.

Additional details and impacted files
@@            Coverage Diff            @@
##           1.0.0dev    #1977   +/-   ##
=========================================
  Coverage     98.72%   98.72%           
=========================================
  Files            35       35           
  Lines          4628     4628           
=========================================
  Hits           4569     4569           
  Misses           59       59           
Flag Coverage Δ
unittests 98.72% <ø> (ø)

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.

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

The development version of Numba 0.62.0 is unhappy about `range`
being applied to floating point values here.
@emilazy
Copy link
Author

emilazy commented Sep 3, 2025

Very reasonable, both on waiting on CI and on the cast. I’ve pushed a much simpler fix to preserve the previous behaviour without changing the types.

@bmcfee bmcfee changed the base branch from main to 1.0.0dev September 4, 2025 15:32
@bmcfee
Copy link
Member

bmcfee commented Sep 4, 2025

Thanks @emilazy - hitting lint errors because the main branch is a bit stale right now. I'm rebasing to the 1.0dev branch just to get past that.

@emilazy
Copy link
Author

emilazy commented Sep 7, 2025

FWIW, it looks like Numba plan to roll back the change due to it also causing issues with integers of mixed signedness. I assume the check for floats will return at some point though (this code doesn’t work in vanilla Python either), so this will still be relevant then.

@bmcfee
Copy link
Member

bmcfee commented Sep 9, 2025

Ok, thanks for the heads up @emilazy.

I guess this is now a fairly innocuous change, but I see no reason to delay merging. Is there anything else to do here for now?

@emilazy
Copy link
Author

emilazy commented Sep 9, 2025

It should be good to go as far as I’m concerned, if you want to hit the button :) Thanks for the review!

@bmcfee bmcfee merged commit 8b92a65 into librosa:1.0.0dev Sep 9, 2025
12 of 15 checks passed
@bmcfee
Copy link
Member

bmcfee commented Sep 9, 2025

Done - thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants