search

LIHPC-Computational-Geometry / metis-rs

Idiomatic wrapper for METIS, the serial graph partitioner and fill-reducing matrix orderer
https://lihpc-computational-geometry.github.io/metis-rs/metis/
Apache License 2.0
11 stars 9 forks source link

Remove mutability requirement on input from public facing API #18

Closed oisyn closed 1 year ago

oisyn commented 1 year ago

The new Rust version seems to have extra checks on casting &T to &mut T. The Rust-facing API just copies METIS' C API, which accepts pointers to non-const, which is why those casts were needed. But now I'm getting all these errors I figured why not fix the API itself.

I went ahead and made all input parameters immutable. In the unsafe blocks where the C API is called is then dealt with casting the *const T to *mut T.

oisyn commented 1 year ago

Wait why did 1.61.0 suddenly start failing? Cargo.lock hasn't changed since the last commit.

EDIT: oh I see why, Cargo.lock is specified in .gitignore, so it downloads the latest version of all crates that fit within the spec of Cargo.toml. That's unfortunate. This basically means that checks can fail at any moment.

UPDATE: A fix for this is made in #20. This will resolve the failing tests for this PR.

hhirtz commented 1 year ago

Thanks. Can you also update /examples/graph.rs and ensure the C numbering is used? We can hide this option then.

Having fortran numbering makes metis mutate xadj and adjncy: https://github.com/KarypisLab/METIS/blob/e0f1b88b8efcb24ffa0ec55eabb78fbe61e58ae7/libmetis/pmetis.c#L116-L120 https://github.com/KarypisLab/METIS/blob/e0f1b88b8efcb24ffa0ec55eabb78fbe61e58ae7/libmetis/fortran.c#L16-L28 https://github.com/KarypisLab/METIS/blob/e0f1b88b8efcb24ffa0ec55eabb78fbe61e58ae7/libmetis/options.c#L87

oisyn commented 1 year ago

Oh good call, I hadn't noticed that. Yeah, let's remove the Fortran numbering, but what do we do with set_options? It basically allows a user to bypass the exposed options.

We could either check on set_options and panic, or do the check in one of the calls that do the actual work and then return a proper error.

hhirtz commented 1 year ago

Hmm. If we go down the "checks" route as planned in #11 then might as well make set_options return a Result.

oisyn commented 1 year ago

Alright, I've placed a panic! in set_options for now. We can return a proper error as part of the work in #11.