add nalgebra::linalg methods #478
No reviewers
Labels
No Milestone
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: M-Labs/nac3#478
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "support_nalgebra"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Adds implementation of:
try_invert_to
https://docs.rs/nalgebra/latest/nalgebra/linalg/fn.try_invert_to.htmlwilkinson_shift
https://docs.rs/nalgebra/latest/nalgebra/linalg/fn.wilkinson_shift.htmlfrom
nalgebra::linalg
.nac3core/codegen/externfns
provides implementation in rust (similar toirrt.cpp
) for functions that will be made available at runtime. These are linked to the main program innac3standalone/demo/run_demo.sh
to allow thenalgebra::linalg
functions to be tested.If the purpose of the DSO is to make the lli tests work, I would just get rid of the latter. The lli tests increase complexity and AFAICT don't really help find bugs.
And the crate should not be named "externfns" but "nalgebra_c" (or similar) and should be part of demo, not core/codegen.
@ -4,6 +4,7 @@ members = [
"nac3ast",
"nac3parser",
"nac3core",
"nac3core/src/codegen/externfns",
Alarm bells should be ringing when you compare this entry to the others.
Again, move this out of nac3core. This is only used by nac3standalone/demo and this is where it should go.
I also doubt that
clang xxy.so
works to link an executable against a DSO. Did you test it? If it doesn't work just make it a static lib.Will move it.
Yes, I tested it and clang does link against DSO. The DSO can also be linked with lli while Static lib will only work with clang, so I had compiled it to DSO instead.
Just call it linalg or similar, this name is too long.
And can it be moved to demo? Does it need to be at the top level cargo workspace?
Yes, will move it
If its removed from top level cargo workspace and introduced in some other workspace (
nac3standalone
here) cargo will not let it build as it considers this as double having two roots to the project. An alternative could be to have thelinalg
crate as another independent workspace which could be build separately.I don't think it needs a workspace, it can be just a crate.
@ -1874,6 +1876,37 @@ impl<'a> BuiltinBuilder<'a> {
}
}
/// Build the functions `try_invert_to` and `wilkinson_shift`
Poorly written comment. If nalgebra methods are added or removed, it will go out of sync.
@ -263,6 +265,8 @@ impl PrimDef {
PrimDef::FunNpLdExp => fun("np_ldexp", None),
PrimDef::FunNpHypot => fun("np_hypot", None),
PrimDef::FunNpNextAfter => fun("np_nextafter", None),
PrimDef::FunTryInvertTo => fun("try_invert_to", None),
Insert blank line to separate from the other numpy stuff.
And please keep the naming scheme consistent. If this is replacing
np.linalg.inv
, it should be callednp_linalg_inv
, nottry_invert_to
.@ -265,2 +267,4 @@
PrimDef::FunNpNextAfter => fun("np_nextafter", None),
PrimDef::FunTryInvertTo => fun("try_invert_to", None),
PrimDef::FunWilkinsonShift => fun("wilkinson_shift", None),
PrimDef::FunSome => fun("Some", None),
(Not a problem coming from this PR)
Shouldn't
Some
be nearOption
? @derppeningYeah it should. I think it's safe to move that over (as long as it doesn't regress any existing tests).
@ -144,0 +145,4 @@
try:
y = np.linalg.inv(x)
x[:] = y
except np.linalg.LinAlgError:
Why can't we just follow the numpy API and raise this exception on failure, instead of returning True/False?
It is possible to raise a Python exception from ksupport.rs.
The implementation of
try_invert_to
innalgebra::linalg
returns True/False depending on whether the operation succeeded or not. I had the function return True/False to match the implementation innalgebra::linalg
.The reference is numpy.
@ -144,0 +149,4 @@
return False
return True
def wilkinson_shift(x):
No corresponding numpy/scipy library function?
And generally I would try to follow
numpy.linalg
and the functions it provides, not implement random stuff from Rust nalgebra.@ -55,3 +58,3 @@
clang -c -std=gnu11 -Wall -Wextra -O3 -o demo.o demo.c
clang -lm -o demo module.o demo.o
clang -lm -o demo module.o demo.o $externfns
Can you really pass a DSO to clang and get it linked in?
Anyway I would just make it a static lib.
The DSO provides the implementation of
nalgebra::linalg
functions at runtime. Without it, thetest_try_invert_to
andtest_wilkinson_shift
test cases in standalone will not work.add nalgebra::linalg methodsto WIP: add nalgebra::linalg methodsf30683578d
toea5f4fc698
@ -0,0 +56,4 @@
let matrix2 = DMatrix::from_row_slice(dim2_0, dim2_1, data_slice2);
let mut result = DMatrix::<f64>::zeros(dim3_0, dim3_1);
matrix1.mul_to(&matrix2, &mut result);
Isn't that already implemented via the
@
operator?Yes it's already implemented. I added this to test the macro calls. Will remove it from the final version.
@ -1838,0 +1879,4 @@
"",
)
.unwrap(),
"0:ValueError",
As I said before, you can raise exceptions from the Rust firmware.
It is better to write this in Rust than in LLVM IR calls.
Ok.
@ -1838,0 +1865,4 @@
// The following constraints must be satisfied:
// * Input must be 2D
// * number of rows should equal number of columns (square matrix)
if cfg!(debug_assertions) {
debug_assertions is for debugging the compiler.
This is performing a user input check.
ea5f4fc698
to8693cb8f6a
Adds implementation of:
np_dot
: Unlikenumpy
where dot product can be used for multiplying matrices, the current implementation is limited to 1D casenp_linalg_matmul
: Same asnp_matmul
implemented by@
symbol currently but handles ValueErrors for incompatible matricesnp_linalg_cholesky
np_linalg_qr
: The resulting matrices are slightly different than the ones from numpy, but does give a valid decompositionnp_linalg_svd
: The resulting matrices are slightly different than the ones from numpy, but does give a valid decompositionnp_linalg_inv
np_linalg_pinv
sp_linalg_lu
sp_linalg_schur
: The resulting matrices are slightly different than the ones from numpy, but does give a valid decompositionsp_linalg_hessenberg
: The resulting matrices are slightly different than the ones from numpy, but does give a valid decompositionWith the exception of
np_dot
all other operations work on 2DNDArray
only. Othernumpy
andscipy
functions likemax
,min
,transpose
,reshape
,lu_solve
etc. can all be similarly implemented using the corresponding functions innalgebra
but will be limited to 1D or 2D arraysWIP: add nalgebra::linalg methodsto add nalgebra::linalg methodsWhy?
What does that means exactly? And why?
@ -1838,0 +1883,4 @@
let (BasicTypeEnum::FloatType(_), BasicTypeEnum::FloatType(_)) = (n1_elem_ty, n2_elem_ty)
else {
unimplemented!("{FN_NAME} operates on float type NdArrays only");
Will this just crash the compiler if the user tries to do it on integer matrices?
See how the other compiler user error reporting is done.
For now, I have specified the numpy element type in
linalg_externfns/src/lib.rs
to bef64
. We can extend this to supporti32
ori64
type as well but will need dedicated functions to handle different types.I think limiting to float is fine. But the compiler should not crash if the user passes integer, and report the error correctly instead.
@ -1838,0 +2033,4 @@
}
}
/// Invokes the `np_linalg_svd` using `nalgebra` crate
What crate is used is a firmware implementation detail. Remove this comment and remove any code that goes along that line.
@ -0,0 +31,4 @@
raise_exn!($name, $fn_name, $message, 0, 0, 0)
}};
}
pub struct InputMatrix {
code formatting
@ -0,0 +8,4 @@
use core::slice;
use nalgebra::DMatrix;
macro_rules! raise_exn {
We don't support exceptions in the host demo and we don't plan to. Just call abort(), assert() and similar. Please keep everything as simple as possible.
Added this in-line with the firmware, so the runtime behavior can be somewhat replicated making it simpler when moving
lib.rs
to artiq-zynq or artiq system. Will replace the exceptions with apanic
statement in standalone then.@ -0,0 +30,4 @@
}
}
pub unsafe fn raise(exception: *const Exception) -> ! {
Does not belong in this PR
Does not belong in a "linalg" crate
Not necessary for the host demo
@ -45,2 +45,4 @@
linalg_externfns=../../target/debug/deps/liblinalg_externfns-?*.a
elif [ -e ../../target/release/nac3standalone ]; then
nac3standalone=../../target/release/nac3standalone
linalg_externfns=../../target/release/deps/liblinalg_externfns-?*.a
Why the globbing?
Doing
cargo build
in standalone directory buildslinalg_externfns
as a dependency and adds a hash (I think its a hash) with its name.Are you sure you're not using the wrong files? IIRC cargo eventually gives you one single .a with a stable name when building a static lib.
Not resolved?
Globbing was needed when linalg was used as dependency of standalone (cargo puts dependencies separately with a hash in front of their names). After delegating the compilation to
run_demo.sh
, there is no longer any need for globbing.@ -17,9 +17,7 @@ demo="$1"
echo -n "Checking $demo... "
./interpret_demo.py "$demo" > interpreted.log
./run_demo.sh --out run.log "${nac3args[@]}" "$demo"
./run_demo.sh --lli --out run_lli.log "${nac3args[@]}" "$demo"
Removing LLI should (1) be done more thoroughly, we don't want to drag dead code along (2) be a separate PR.
Since we can't really link a static library with
lli
it won't be able to run the new functions and the tests will fail. Generating DSO and linking using that should work though.Yes, let's just remove the LLI tests entirely.
When performing dot product on 1D vectors, output is a single float while a 2D matrix needs to be allocated with 2D inputs. The return type in two cases was different. The
numpy
documentation recommends usingmatmul
for 2D case anyways and since it was already implemented, I though it would be fine to havenp_dot
calculate inner product alone.Matrix decompositions that give two matrices and don't have unique decompositions had some of their entries oppositely signed. An issue was raised on the
nalgebra
for QR discrepancies (https://github.com/dimforge/nalgebra/issues/735) but their fix doesn't really work. I am not sure about the reason for the discrepancy as usually one of the rows (and corresponding column of the other matrix) will have its sign reversed. The decomposition is still valid though, so I don't think it will pose an issue with actual usage.8693cb8f6a
to688e85d13c
@ -63,0 +66,4 @@
# Compile linalg crate to provide functions compatible with i386 architecture
if [ ! -f ../../target/i686-unknown-linux-gnu/release/liblinalg.a ]; then
cd linalg && nix-shell -p rustup --command "RUSTFLAGS=\"-C target-cpu=i386 -C target-feature=+sse2\" cargo build -q --release --target=i686-unknown-linux-gnu" && cd ..
Since we are testing on i386 as well, had to build an alternative static lib for the new target for linking.
I don't think you need to use rustup for that. Additionally, rustup will not work on Hydra.
If this is fixing rustc whining about "you need to use nightly toolchain for bla bla bla", just shut it up with RUSTC_BOOTSTRAP=1.
Only the
x86_64
version libstd was provided by default in the toolchain, so usedrustup
to build the static lib for i386 architecture.Tried building the libstd using
-Zbuild-std
, but that still requiredrust-src
from rustup. I didn't find a workaround that could avoid usingrustup
, so added the compiled static lib file for both architectures along this PR.Look here for a full workaround:
https://git.m-labs.hk/M-Labs/kirdy/src/branch/master/BSD.md
Committing binaries to the repository is not acceptable.
Moved
linalg
functions undernac3standalone/demo
and updated the comments and error reporting.08a5b74ae6
to4a6845dac6
Reimplemented simple functions (
np_transpose
andnp_dot
) using LLVM IR and added additional functions (np_linalg_matrix_power
,np_linalg_det
).@ -1839,0 +1867,4 @@
out_ptr
}
/// Invokes the `np_linalg_matmul` linalg function
remove
@ -102,0 +114,4 @@
FunNpLinalgDet,
FunSpLinalgLu,
FunSpLinalgSchur,
FunSpLinalgHessenberg,
Bit weird to have those complicated matrix ops before simple functions like round() in the list.
Probably better addressed in another PR though.
@ -102,0 +104,4 @@
// Linalg functions
FunNpDot,
FunNpLinalgMatmul,
I think having only
@
is fine.linalg.matmul is probably from before Python introduced
@
.@ -8,6 +8,7 @@ edition = "2021"
parking_lot = "0.12"
nac3parser = { path = "../nac3parser" }
nac3core = { path = "../nac3core" }
linalg = { path = "./demo/linalg" }
This is part of the demo and the demo only. Shouldn't the demo runner build that part, instead of building it when nac3standalone is built?
Just like the demo runner is already building the glue C code.
Will delegate its compilation to
run_demo.sh
similar to the current approach when testing with -i386.@ -5,6 +5,7 @@ import importlib.util
import importlib.machinery
import math
import numpy as np
import scipy as sp
Move one line below.
To keep the numpy imports together.
f954887acf
to2237137f1a
2237137f1a
to63d2b49b09
The standalone tests with -i386 will fail currently as
linalg
function compilation toi386
is left out (@ -64,2 +64,2 @@
clang -m32 -c -std=gnu11 -Wall -Wextra -O3 -msse2 -o demo.o demo.c
clang -m32 -lm -Wl,--no-warn-search-mismatch -o demo module.o demo.o
cd linalg && cargo build --release --target i686-unknown-linux-gnu -q && cd ..
indentation
But nevermind I'll fix it