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

Acceleration || branching from #68 #238

Open
bmcfee opened this issue Feb 24, 2017 · 13 comments
Open

Acceleration || branching from #68 #238

bmcfee opened this issue Feb 24, 2017 · 13 comments
Labels

Comments

@bmcfee
Copy link
Collaborator

bmcfee commented Feb 24, 2017

The issue of acceleration came up in #68 , either by numba or by cython. It seems like we should branch the discussion off into its own issue.

Cython

  • Pro: minimal dependencies, pip installable, wheels
  • Con: packaging can be tricky, significant developer time required

Numba

  • Pro: minimal developer time required
  • Con: not pip installable due to LLVM dependency

My two cents: despite the dependency chain problem, I'd prefer numba over cython because it would be easier to deploy across the entire module.

In librosa, we handled this by making the numba dependency optional, and providing a dummy decorator optional_jit that does a no-op if numba is not available, and jit-compilation if it is. That way, everything still works, but if you want to get the acceleration benefits, it's on you to install numba.

Side note: if we also add conda(-forge) package distribution, we can easily add numba as a hard dependency there.

What do y'all think?

@justinsalamon
Copy link
Collaborator

Side note: if we also add conda(-forge) package distribution, we can easily add numba as a hard dependency there.

+1!

@craffel
Copy link
Owner

craffel commented Feb 27, 2017

What part of mir_eval is too slow? I think most of the metrics are reasonably fast, and those that aren't could probably benefit most from some regular Python hand-optimization. I don't think it's worth considering Cython, given the goal of "a transparent implementation". numba would be fine with the optional jit decorator.

@bmcfee
Copy link
Collaborator Author

bmcfee commented Feb 27, 2017

What part of mir_eval is too slow?

The separation module is particularly slow. Yes, it could benefit from a thorough efficiency audit -- I've noticed some pretty obvious examples of redundant computation -- but I think a lot of its slowness comes from tight-loop calculations that don't trivially vectorize, but would be easily handled by numba.

Some of the segmentation / hierarchy metrics could also benefit from optimization.

I haven't benchmarked the rest of it, but those alone seem worth the effort to me.

@craffel
Copy link
Owner

craffel commented Feb 27, 2017

I think as a first pass it's worth looking in detail at what metrics are prohibitively slow and if there's an easy way to make them faster without numba (since we can assume for the time being most users will not have numba installed). Adding numba + optional_jit seems like a no-brainer after that.

@bmcfee
Copy link
Collaborator Author

bmcfee commented Feb 27, 2017

Sure.

Side note: is it worth making up a separate issue to put mir_eval in conda-forge?

@craffel
Copy link
Owner

craffel commented Feb 27, 2017

Side note: is it worth making up a separate issue to put mir_eval in conda-forge?

Sure, wouldn't hurt. I would prioritize it based on how much people are complaining about it not being in conda-forge.

@stefan-balke
Copy link
Contributor

:complain:

😄

@craffel
Copy link
Owner

craffel commented Feb 28, 2017

About which functions?

@bmcfee
Copy link
Collaborator Author

bmcfee commented Jul 13, 2017

Btw, numba now has wheels on pypi, so we probably could make it a hard dependency.

@bmcfee
Copy link
Collaborator Author

bmcfee commented Sep 8, 2017

Update: llvmlite >=0.20 (now up) ships with windows wheels.

@SunDoge
Copy link

SunDoge commented Oct 23, 2019

The separation metrics are too slow!!! It takes up all the time to compute the score when I was training my model. Could you please add jit to those eval functions, e.g. bss_eval_sources.

@bmcfee
Copy link
Collaborator Author

bmcfee commented Oct 23, 2019

Could you please add jit to those eval functions

I'm not sure that jitting would help for bss eval. Last I checked, the bottleneck here was in computing a bunch of frame-wise least squares solutions, and that part is already calling out to C under the hood (via numpy/scipy).

But do feel free to try it out and send a PR if it improves.

@faroit
Copy link

faroit commented Oct 23, 2019

I'm not sure that jitting would help for bss eval. Last I checked, the bottleneck here was in computing a bunch of frame-wise least squares solutions, and that part is already calling out to C under the hood (via numpy/scipy).

Can confirm @bmcfee I tried to to strip out some parts for numba and got a bit more than 5% speedup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants