sparsemat / sprs

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

Split the error type between structural and linalg errors #268

Closed vbarrielle closed 3 years ago

vbarrielle commented 3 years ago

Recent work has made the structural constraints of sparse matrices and vectors clearer, making it clear that there can be a single enum discriminating all possible structural errors. Having linalg errors in the same error type was clumsy: structural errors happen when creating a matrix, while linalg errors happen much later, when solving systems or performing other linear algebra operations. Therefore, by splitting the types, it should be easier to react to both kinds of errors.

Since the linalg part of this crate is still small, it's expected that I'm missing possible errors, which is why the error type is a non exhaustive enum.

Additionaly, I took this refactoring as an opportunity to fix #255, using the same pattern used by the CsMatBase and CsVecBase constructors to return the storage in case of an error.

@mulimoen I'm very interested in your remarks on the design of these new error types.

mulimoen commented 3 years ago

Looking at this on my phone it looks great! Will have access to my computer later this week for a proper look.

Would it be an idea to have some error-type which is an enum (SprsError) of these two for end-users? Would make migration very easy

vbarrielle commented 3 years ago

Would it be an idea to have some error-type which is an enum (SprsError) of these two for end-users? Would make migration very easy

I'm not sure how it would help migration. Constructors will return a StructureError, so I don't see how the user would get an enum (SprsError) of LinalgError and StructureError.

Something that could be done to ease migration, would be to have a type alias pub type SprsError = StructureError, which would mean constructors would not need much migration, but linalg functions would. But since the variants have changed, it's not a guarantee there would be no breakage.

mulimoen commented 3 years ago

I was thinking something along this:

#[derive(PartialEq, Debug, Clone)]
#[non_exhaustive]
/// Convenience type for errors that may occur in this crate.
/// All other error types can be converted into this
/// by using `Into` or `From`
pub enum SprsError {
    Structure(StructureError),
    Linalg(LinalgError),
}

impl From<StructureError> for SprsError {
    fn from(e: StructureError) -> Self {
        Self::Structure(e)
    }
}

impl From<LinalgError> for SprsError {
    fn from(e: LinalgError) -> Self {
        Self::Linalg(e)
    }
}

impl std::fmt::Display for SprsError {
    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
        match self {
            Self::Structure(e) => write!(f, "{}", e),
            Self::Linalg(e) => write!(f, "{}", e),
        }
    }
}

impl std::error::Error for SprsError {}

For migration one could use map_err(|e| e.into()) and retain SprsError as an error from the sprs crate. Kind of a catch-all error type which we won't return from our methods.

vbarrielle commented 3 years ago

Ok I get it now, looks nice indeed, I'll add it. Thanks!

vbarrielle commented 3 years ago

@mulimoen the SprsError type you suggested is now present.

mulimoen commented 3 years ago

I had another look, and the PR seems ready to merge. Excellent work @vbarrielle, will make future error handling a breeze