Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft: Support generic dense vectors in more APIs #272

Merged
merged 7 commits into from
Feb 15, 2021
Merged

Conversation

vbarrielle
Copy link
Collaborator

Lots of APIs were using slices as their input, particularly when using
output buffers. However, this was not a good fit for linear algebra,
since consumers of the APIs are more likely to be using eg an array from
ndarray.

Here we extend the DenseVector trait with a mutable version to be able
to use it on output parameters. Since these are sealed traits we should
be able to add unsafe indexing if necessary without a breaking change.
It should also be possible to support eg nalgebra in the future without
too much trouble.

This should improve the situation discussed in #93, though it's
probably not done yet.

@vbarrielle vbarrielle marked this pull request as draft January 20, 2021 22:36
@vbarrielle
Copy link
Collaborator Author

This will probably require some iteration. @mulimoen I'd be very interested in your feedback :)

@vbarrielle vbarrielle added this to the 0.10 milestone Jan 20, 2021
@mulimoen
Copy link
Collaborator

This looks great! I tried implementing DenseVector on Deref<Target = [N]> to replace some of the boilerplate implementations, but this seems to clash with ndarray::ArrayBase, even though this does not implement Deref. It seems rust has some forward protection based on the error message:

note: upstream crates may add a new impl of trait `std::ops::Deref` for type `ndarray::ArrayBase<_, ndarray::Dim<[usize; 1]>>` in future versions

@mulimoen
Copy link
Collaborator

I was thinking about incorporating the nshare crate, but I don't think this is a good idea. Adding support for nalgebra in the new traits is easy, and we would know there is no overhead. Outside crates should probably implement DenseVector themselves, without going through nshare.

src/dense_vector.rs Outdated Show resolved Hide resolved
src/dense_vector.rs Outdated Show resolved Hide resolved
@vbarrielle
Copy link
Collaborator Author

This looks great! I tried implementing DenseVector on Deref<Target = [N]> to replace some of the boilerplate implementations, but this seems to clash with ndarray::ArrayBase, even though this does not implement Deref.

Yes orphan rules will prevent us from having too broad implementations here, I think we need to implement on a case by case. Fortunately most implementations are trivial.

I was thinking about incorporating the nshare crate, but I don't think this is a good idea. Adding support for nalgebra in the new traits is easy, and we would know there is no overhead. Outside crates should probably implement DenseVector themselves, without going through nshare.

Right now the DenseVector trait is not publicly exposed by sprs, it is sealed. Which is a nice way for us to control precisely the types we want to support. I do think we want to support nalgebra better in the future, though probably with a feature flag to control the support.

I haven't played with nshare yet, so I don't know if it's convenient to use.

@vbarrielle vbarrielle force-pushed the dense_vector branch 3 times, most recently from 347b5f0 to 280f276 Compare January 25, 2021 21:35
Lots of APIs were using slices as their input, particularly when using
output buffers. However, this was not a good fit for linear algebra,
since consumers of the APIs are more likely to be using eg an array from
ndarray.

Here we extend the `DenseVector` trait with a mutable version to be able
to use it on output parameters. Since these are sealed traits we should
be able to add unsafe indexing if necessary without a breaking change.
It should also be possible to support eg nalgebra in the future without
too much trouble.

This should improve the situation discussed in #93, though it's
probably not done yet.

As suggested by @mulimoen, the `index` and `index_mut` implementations
hint as `#[inline(always)]` as we want them to be zero-cost
abstractions, and it can be critical to have them inlined to allow the
compiler to remove bounds checks when possible.
@vbarrielle vbarrielle force-pushed the dense_vector branch 2 times, most recently from 780241b to c25bd31 Compare January 27, 2021 21:21
@vbarrielle
Copy link
Collaborator Author

vbarrielle commented Jan 27, 2021

With the latest changes, I think I'm leaning towards an interesting implementation, I've been able to write a single product impl allowing to apply a permutation to any dense vector (so a Vec, a slice, or an ndarray::Array).

I'm starting to think the DenseVector trait should be exposed outside the crate, I feel the need for it to make the solve in sprs-ldl more generic:

    //I'd like to use DenseVector here, but I probably need to expose it for public use
    pub fn solve<'a, V>(&self, rhs: &V) -> Vec<N>
    where
        N: 'a + Copy + Num,
        V: Deref<Target = [N]>,
    {
        let mut x = &self.symbolic.perm * &rhs[..];
        let l = self.l();
        ldl_lsolve(&l, &mut x);
        linalg::diag_solve(&self.diag, &mut x);
        ldl_ltsolve(&l, &mut x);
        let pinv = self.symbolic.perm.inv();
        &pinv * &x
    }

There's another interesting opportunity with this refactor: if I manage to implement everything that's related to dense vectors using these traits, then I can make the ndarray dependency optional (but on by default I guess), and also add an optional dependency to nalgebra that would also implement this trait. Which is probably not enough, as I'd need also a trait to represent dense matrices to convert from sparse to dense, but it looks like an interesting future direction.

Now it's possible to call on ndarray data as well, with a single impl.
This way dependent crates will be able to express algorithms in terms of
that trait. It will remain sealed initially, as we want to gain
experience before committing on its API.
This removes some code duplication, and benchmarks show this does not
affect performance.

Also pointed to a place where I'm not satisfied with the current API,
but where I don't see how to improve with the current rust's trait
system.
@vbarrielle vbarrielle marked this pull request as ready for review February 15, 2021 21:01
@vbarrielle
Copy link
Collaborator Author

So I think I've reached a good state, I've been able to use DenseVector in all required places. @mulimoen if you're OK with the current state then I'll merge.

Copy link
Collaborator

@mulimoen mulimoen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a massive boost in usability of this crate! Looks great, I say this is ready to be merged.

@vbarrielle
Copy link
Collaborator Author

Thanks for the review @mulimoen !

@vbarrielle vbarrielle merged commit e857e1d into master Feb 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants