search

sparsemat / sprs

sparse linear algebra library for rust
Apache License 2.0
386 stars 45 forks source link

FillInReduction enum is not a good API for version stability #214

Closed vbarrielle closed 3 years ago

vbarrielle commented 4 years ago

This enum lists all existing fill in reduction methods in the sprs ecosystem. I realized while binding such methods from suitesparse that each time a method would be added, this would require changing this enum and thus making a breaking change. This does not look like a good idea, so this enum should probably go away in the future.

mulimoen commented 4 years ago

An alternative is to use #[non_exhaustive] on the affected enum

vbarrielle commented 4 years ago

Yes, I've been considering that as well. However I'm not sure if it's a good idea to expose this enum at all. It is used in LDL decomposition to specify decomposition options in a builder pattern (https://github.com/vbarrielle/sprs/blob/master/sprs-ldl/src/lib.rs#L132-L143), but there may be more idiomatic ways to do it (eg by passing a FnOnce that will perform the decomposition when the input matrix is passed). That way no need to match on all possible enums, and it's extendable with other crates as well.

vbarrielle commented 3 years ago

Fixed by #223, I've chosen to use #[non_exhaustive], passing a function would have made the API too cumbersome (right now one nice thing about the Ldl builder struct is that it does not depend on the actual type of the matrix up until the last step, and this property would have been lost).