bonevbs / HssMatrices.jl

A Julia package for hierarchically semiseparable (HSS) matrices.
MIT License
21 stars 5 forks source link

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

Closed BaptisteLamic closed 1 year ago

BaptisteLamic commented 3 years ago

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.

bonevbs commented 3 years ago

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 :)

BaptisteLamic commented 3 years ago

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.

codecov[bot] commented 3 years ago

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 ```diff @@ 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](https://codecov.io/gh/bonevbs/HssMatrices.jl/pull/15?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Boris+Bonev) | Coverage Δ | | |---|---|---| | [src/compression.jl](https://codecov.io/gh/bonevbs/HssMatrices.jl/pull/15?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Boris+Bonev#diff-c3JjL2NvbXByZXNzaW9uLmps) | `97.34% <100.00%> (+2.29%)` | :arrow_up: | | [src/generators.jl](https://codecov.io/gh/bonevbs/HssMatrices.jl/pull/15?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Boris+Bonev#diff-c3JjL2dlbmVyYXRvcnMuamw=) | `70.66% <100.00%> (+18.66%)` | :arrow_up: | | [src/hssmatrix.jl](https://codecov.io/gh/bonevbs/HssMatrices.jl/pull/15?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Boris+Bonev#diff-c3JjL2hzc21hdHJpeC5qbA==) | `70.73% <100.00%> (+15.01%)` | :arrow_up: | | [src/matmul.jl](https://codecov.io/gh/bonevbs/HssMatrices.jl/pull/15?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Boris+Bonev#diff-c3JjL21hdG11bC5qbA==) | `91.89% <100.00%> (ø)` | | | [src/ulvdivide.jl](https://codecov.io/gh/bonevbs/HssMatrices.jl/pull/15?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Boris+Bonev#diff-c3JjL3VsdmRpdmlkZS5qbA==) | `97.31% <100.00%> (+0.01%)` | :arrow_up: | | [src/ulvfactor.jl](https://codecov.io/gh/bonevbs/HssMatrices.jl/pull/15?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Boris+Bonev#diff-c3JjL3VsdmZhY3Rvci5qbA==) | `94.28% <100.00%> (ø)` | | | [src/HssMatrices.jl](https://codecov.io/gh/bonevbs/HssMatrices.jl/pull/15?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Boris+Bonev#diff-c3JjL0hzc01hdHJpY2VzLmps) | `94.11% <0.00%> (-2.66%)` | :arrow_down: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Boris+Bonev). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Boris+Bonev)

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

BaptisteLamic commented 3 years ago

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 commented 1 year ago

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

bonevbs commented 1 year ago

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.

BaptisteLamic commented 1 year ago

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

BaptisteLamic commented 1 year ago

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)

bonevbs commented 1 year ago

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 commented 1 year ago

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