sarah-ek / faer-rs

Linear algebra foundation for the Rust programming language

https://faer-rs.github.io

MIT License

1.75k stars 56 forks source link

Small improvement for pseudoinverse #122

Closed kenken-neko closed 3 months ago

kenken-neko commented 3 months ago

Change

The process of reading singular values twice (the two processes below) in the implementation of compute_pseudoinverse was reduced to once.

Process to determine if the singular value is less than or equal to the epsilon
Process to calculate the inverse of the singular value

Reason

To reduce the processing time by reducing the number of readings.

sarah-ek commented 3 months ago

did you run any benchmarks comparing before and after? reading the diagonal is by far the least time consuming part of the pseudoinverse computation

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 85.99%. Comparing base (b014e06) to head (ead917e).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #122 +/- ## ========================================== - Coverage 86.03% 85.99% -0.04% ========================================== Files 125 125 Lines 89659 89662 +3 ========================================== - Hits 77135 77105 -30 - Misses 12524 12557 +33 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kenken-neko commented 3 months ago

@sarah-ek No comparison was made because the current benchmark test does not have an compute_pseudoinverse implementation. I made this PR because I believe it is obvious that any short process reduces processing time.

It will take some time to implement the benchmark due to my work. I will submit review request once the benchmark comparison is complete.

kenken-neko commented 3 months ago

I am temporarily closing this PR because I do not know when I will be able to implement the benchmark.