search

urinieto / msaf

Music Structure Analysis Framework
MIT License
478 stars 79 forks source link

[Refactorings]Few code optimizations #117

Closed maldil closed 5 months ago

maldil commented 2 years ago

The Numpy API documentation recommends using np.linalg.multi_dot instead of multiple np.dot calls since it is more succinct and effective. This PR is to make the required changes.

urinieto commented 5 months ago

@carlthome can you help me understand why the pre-commit is failing here? Thanks

carlthome commented 5 months ago

Looks like black wants to apply changes:

https://github.com/urinieto/msaf/actions/runs/7483957687/job/20370019461#step:4:104

urinieto commented 5 months ago

How can I allow black to apply such changes?

carlthome commented 5 months ago

How can I allow black to apply such changes?

Ah, do you mean as in automatically having GitHub Actions make a robot commit with style fixes to the PR?

Think I could get that setup if you'd like (let me propose a workflow change in a separate PR)!

Otherwise, the conservative workflow is to manually notice the CI build log errors, and to apply black locally with a subsequent git commit; git push dance.

With pre-commit specifically, the idea is that mistakes are caught by doing pre-commit install once in the local repo, at which point git commit will run black etc. automatically. pre-commit uninstall would disable the behaviour and pre-commit run would run on-demand instead.

PS: Some people have strong opinions against automating git commit on CI but personally I think it's pretty nice for these sorts of automatic code formatting things!

urinieto commented 5 months ago

Gotcha @carlthome . It'd be nice to have black format external code automatically, but only if you think that's safe enough. I just followed your suggestions and everything seemed to work. Thanks!

Also, @maldil thanks for your contributions! Apologies this took so long. This all looks good, even though I wonder how much speed you get from multi_dot vs dot. In any case, good stuff!