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

[Refactorings]Few code optimizations #117

Merged
merged 5 commits into from
Jan 13, 2024
Merged

[Refactorings]Few code optimizations #117

merged 5 commits into from
Jan 13, 2024

Conversation

maldil
Copy link
Contributor

@maldil maldil commented Jul 6, 2022

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
Copy link
Owner

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

@carlthome
Copy link
Contributor

Looks like black wants to apply changes:

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

@urinieto
Copy link
Owner

How can I allow black to apply such changes?

@carlthome
Copy link
Contributor

carlthome commented Jan 12, 2024

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
Copy link
Owner

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!

@urinieto urinieto merged commit e151322 into urinieto:main Jan 13, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants