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

Allocations using mul! #109

Open
jlchan opened this issue Oct 14, 2021 · 5 comments
Open

Allocations using mul! #109

jlchan opened this issue Oct 14, 2021 · 5 comments

Comments

@jlchan
Copy link

jlchan commented Oct 14, 2021

Thanks for this package! I've noticed there are some allocations when using mul!(y,A,x), where x,y are <: Vector and A is a KroneckerProduct. This seems to be due to alloc_temp_array in _kron_mul_fast_square!(C, B, factors).

Since we're calling mul! many times, these allocations add up. Would it make sense to have a version of KroneckerProduct which stores this temporary array as a hidden field to avoid allocating?

@MichielStock
Copy link
Owner

I think TensorOperations does something like that.

I am a bit hesitant to allow hidden matrices to be generated for this package, though it might make sense to have versions of mul! where users can provide a temp matrix?

@jlchan
Copy link
Author

jlchan commented Oct 16, 2021

Thanks for the note on TensorOperations.jl. A version of mul! with a temporary array argument also makes sense. I'll take a look at TensorOperations.jl, but if Kronecker.jl fits better I'm happy to try a PR for the new mul!.

@MichielStock
Copy link
Owner

Kronecker is likely easier to use if you are just doing matrix algebra. TO is a bit more general and advanced and can likely attain somewhat higher performance. Take a look and share your thoughts if we should extend Kronecker in this direction.

@jlchan
Copy link
Author

jlchan commented Oct 18, 2021

After looking at TO, I think Kronecker.jl would be a better fit. I can work on a PR for the temporary array argument mul!, or if you have some ideas I'm happy to try to implement them.

@MichielStock
Copy link
Owner

Maybe you can best take the lead in this and we can look together if it can be improved. Feel free to open a PR!

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

No branches or pull requests

2 participants