HssMatrices.jl icon indicating copy to clipboard operation
HssMatrices.jl copied to clipboard

Add type information to fix HssMatrix{T} when T is not a Float64

Open BaptisteLamic opened this issue 4 years ago • 4 comments

Several functions were generating errors when called with HssMatrix{T} where T was not a Float64. These include: \ , / and compress_adatative.

This commit solves this problem and adds a test for it.

BaptisteLamic avatar May 05 '21 14:05 BaptisteLamic

Hi Baptiste, thanks for contributing! I must admit HssMatrices.jl was written in quite a rush as I needed something to work with.

I will try to have a look soon so we can merge it :)

bonevbs avatar May 05 '21 19:05 bonevbs

You are welcome! That’s a nice library. The test failed, but they were passing on my lab computer. It may due to the use of MKL instead of OpenBLAS, I will try to get them pass with the latter.

BaptisteLamic avatar May 06 '21 09:05 BaptisteLamic

Codecov Report

Base: 71.38% // Head: 76.15% // Increases project coverage by +4.76% :tada:

Coverage data is based on head (3c80110) compared to base (980bce6). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #15      +/-   ##
==========================================
+ Coverage   71.38%   76.15%   +4.76%     
==========================================
  Files          12       12              
  Lines        1052     1061       +9     
==========================================
+ Hits          751      808      +57     
+ Misses        301      253      -48     
Impacted Files Coverage Δ
src/compression.jl 97.34% <100.00%> (+2.29%) :arrow_up:
src/generators.jl 70.66% <100.00%> (+18.66%) :arrow_up:
src/hssmatrix.jl 70.73% <100.00%> (+15.01%) :arrow_up:
src/matmul.jl 91.89% <100.00%> (ø)
src/ulvdivide.jl 97.31% <100.00%> (+0.01%) :arrow_up:
src/ulvfactor.jl 94.28% <100.00%> (ø)
src/HssMatrices.jl 94.11% <0.00%> (-2.66%) :arrow_down:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov[bot] avatar May 06 '21 19:05 codecov[bot]

I have tried running the tests without the use of eps(real(T)). When I remove it from the line that defines HssMatrix rtol and atol, the tests still pass. But if I remove it from test line 41, the test fails for ComplexF32, the error magnitude is 1E-8, which is close to eps(Float32).

BaptisteLamic avatar May 11 '21 10:05 BaptisteLamic

Hi, this last version seems ready to be integrated for me. It would be nice that you could have a look.

BaptisteLamic avatar Dec 22 '22 08:12 BaptisteLamic

Hi Baptiste, apologies for the slow replies. I have been quite busy with some other projects.

I just had a look, the code looks mostly good to me - please have a look at the comment I made.

bonevbs avatar Dec 22 '22 08:12 bonevbs

Hi, I just push a commit to add tests for random access. I will quickly have a look for performance regression.

BaptisteLamic avatar Dec 23 '22 06:12 BaptisteLamic

I just run the benchmarks on an Apple Silicon. There are no notable differences.

New results Benchmarking full... 13.976 ms (738 allocations: 70.91 MiB) Benchmarking getindex... 101.208 μs (1100 allocations: 711.03 KiB) Benchmarking compression... 124.240 ms (7775 allocations: 438.17 MiB) Benchmarking randomized compression... 34.689 ms (9514 allocations: 65.64 MiB) Benchmarking re-compression... 2.274 ms (9150 allocations: 4.82 MiB) Benchmarking proper... 3.309 ms (3946 allocations: 6.40 MiB) Benchmarking addition... 199.000 μs (545 allocations: 4.90 MiB) Benchmarking matvec... 340.625 μs (581 allocations: 1.47 MiB) Benchmarking ulvfactsolve... 8.547 ms (3259 allocations: 22.62 MiB) Benchmarking hssldivide... 32.619 ms (43121 allocations: 72.04 MiB)

Old results Benchmarking full... 14.281 ms (738 allocations: 70.91 MiB) Benchmarking getindex... 106.583 μs (1100 allocations: 711.03 KiB) Benchmarking compression... 135.512 ms (7775 allocations: 438.17 MiB) Benchmarking randomized compression... 34.507 ms (9514 allocations: 65.64 MiB) Benchmarking re-compression... 2.213 ms (9150 allocations: 4.82 MiB) Benchmarking proper... 3.282 ms (3946 allocations: 6.40 MiB) Benchmarking addition... 198.875 μs (545 allocations: 4.90 MiB) Benchmarking matvec... 345.667 μs (581 allocations: 1.47 MiB) Benchmarking ulvfactsolve... 8.623 ms (3259 allocations: 22.62 MiB) Benchmarking hssldivide... 32.173 ms (43121 allocations: 72.04 MiB)

BaptisteLamic avatar Dec 23 '22 07:12 BaptisteLamic

Thank you for testing the perf regression and all your work. I currently am quite busy and won't have time to check. Once you address my comment let us merge this :)

bonevbs avatar Dec 23 '22 09:12 bonevbs

thank you for taking care of this and apologies for the slow review. it's merged :)

bonevbs avatar Jan 27 '23 13:01 bonevbs