Skip to content

Conversation

@adomasbaliuka
Copy link

@adomasbaliuka adomasbaliuka commented Jun 9, 2023

For performing many Toeplitz-vector products using the same matrix, it is efficient to reuse the factorization of the Toeplitz matrix. The README suggests that this is intended, however it's not documented how to do it. I found out (1, 2) that this works:

using ToeplitzMatrices, LinearAlgebra, Test
T = Toeplitz([0.;randn(499)], [0.;randn(999)])
x = randn(1000)
Tfac = factorize(T)
result = zeros(Float64, 500); @test mul!(result, Tfac, x, 1., 0.) == T*x

The method facilitating this is

mul!(y::StridedVector{T} where T, A::ToeplitzMatrices.ToeplitzFactorization, x::StridedVector{T} where T, α::Number, β::Number) @ ToeplitzMatrices ~/.julia/packages/ToeplitzMatrices/5gW1c/src/linearalgebra.jl:42

Besides being undocumented, it modifies the temp buffer inside the factorization A. This doesn't invalidate the factorization object (because the buffer is "temporary", I guess) but the method is not thread-safe. I think the way to make it thread-safe is to make a copy of the tmp buffer and thus not changing it. However, this obviously has an unwanted performance overhead and I don't know if thread-safety of the method (is it internal? is exporting it intented) is desired. I also didn't look at other similar methods.

I suggest possible measures including

  1. Make this mul! method thread safe, perhaps controlled by a parameter making it thread-safe by default but avoiding the copy when the factorization is not being reused
  2. Add an operator Base.:*(::ToeplitzFactorization, ::StridedVector) that is thread-safe.

I implemented a suggestion for measure 2. I added comments indicating how one may approach measure 1. What do you think?

The code is not ready to merge, it has a lot of comments that should be removed and additional unit tests that illustrate my points, rather than properly testing functionality.

Tests contain some superfluous stuff for illustrating what I'm doing! DO not merge this! Should be changed and rebased.
Because Github CI doesn't have multithreading
@codecov
Copy link

codecov bot commented Jun 9, 2023

Codecov Report

Patch coverage: 94.00% and project coverage change: -0.09 ⚠️

Comparison is base (49bde1d) 94.98% compared to head (e008b1d) 94.90%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
- Coverage   94.98%   94.90%   -0.09%     
==========================================
  Files           7        7              
  Lines         798      844      +46     
==========================================
+ Hits          758      801      +43     
- Misses         40       43       +3     
Impacted Files Coverage Δ
src/ToeplitzMatrices.jl 87.50% <81.81%> (-4.81%) ⬇️
src/linearalgebra.jl 92.67% <96.15%> (+0.28%) ⬆️
src/hankel.jl 96.20% <100.00%> (+0.04%) ⬆️
src/special.jl 99.33% <100.00%> (+0.03%) ⬆️
src/toeplitz.jl 97.56% <100.00%> (+0.03%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@adomasbaliuka
Copy link
Author

I don't know what's wrong with the v1.0.5, I tried starting the package from juliaup's v1.0.5 channel but that errored on instantiate already (ERROR: Unsatisfiable requirements detected for package StatsBase) ...

@putianyi889
Copy link
Contributor

hopefully #91 will help

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.

2 participants