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

WIP: Adds operator Base.:*(::ToeplitzFactorization, ::StridedVector) #103

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

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.

None yet

2 participants