Closed aentity closed 7 months ago
sorry, im in the middle of a big refactor at the moment. so i'd rather not merge pull requests until #107 is merged. but this seems like a nice contribution!
ok @sarah-ek thank you for consideration. please let me know when finished, and i will attempt rebase and fix.
it's merged now. it might take a bit more than a rebase though :sweat_smile:
thank you it was not bad :)) i like addition of ColRef it is nice.
looks mostly good to me. just have a few comments on it
SINGLE_VALUE_RCOND: f64 = 1e-15
, this should be replaced by E::Real::faer_epsilon()
compute_pseudo_inverse
, it would be better to instead use the library-provided implementation. either using *
or crate::linalg::matmul::matmul
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 86.56%. Comparing base (
b62d42f
) to head (ddba518
). Report is 7 commits behind head on main.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
hello, i have fixed 1 and 3, thank you. i am unfamiliar with faer to understand how to fix 2. do you suggest to replace:
let val = vt.read(k, i).faer_mul(u.read(j, k)).faer_div(sv_k);
unsafe { ai.write_unchecked(i, j, ai.read(i, j).faer_add(val)) };
or some other part? thank you
i have pushed two suggestions, thank you, i will await suggestion for how to integrate matrix mul. i do not see way to use if condition.
the idea is to compute V S.+
first, then multiply that by U.T
, rather than trying to compute V S.+ U.T
all at once
how would addition and multiplication handle the branch?
if sv_k > sv_tolerance {
// do fused add mult
}
the branch would be done in the V S.+
multiplication, by replacing the entries of S.+
that would be too large by 0
i'm sorry i need more help than suggestion, or if you know quick answer, please post or edit. i have only made this pr because i need to compute pseudo inverse for ambisonic speaker decoder calculations, and pure rust solution is ideal.
in that case, would it be fine if we merged this PR as is, then i applied the fix myself?
in that case, would it be fine if we merged this PR as is, then i applied the fix myself?
yes i would be very happy. only thing is the tests i added, i know they are correct values, so as long as doesn't alter tests, everything is ok. but i think you know much more what is correct and what is expected here than me :))
also i rebased on main and fixed compile error, hopefully tests should compile now.
one last thing, would you be fine with changing the license to MIT? i would prefer to not mix multiple licenses in one project if i can help it
yes i will of course add whatever license you prefer. i just copied license doc comment from jacobi.rs in svd mod which has this license. in most recent push, i simply deleted license header if ok for you.
that's fine, yes. the jacobi.rs header has that license because it was copied from eigen if i recall correctly
I don't know faer codebase or linear algebra well so I try to do my best. please let me know for corrections, and if unsafe is ok. thank you for excellent library