Add conversions from nalgebra types

[jturner314]

This isn't ready to merge yet because building the docs for nalgebra results in an internal compiler error (rustsim/nalgebra#482). I'd like to wait until this is fixed because I don't want to break ndarray's docs. [Edit: This is now fixed.]

The primary motivation for this is to enable ndarray-linalg to use nalgebra as a backend for linear algebra operations when BLAS/LAPACK aren't available (termoshtt/ndarray-linalg#121).

Other notes:

• I've implemented a conversion to a 1-D array only for column vectors (shape n×1). It would be nice to also allow row vectors (shape 1×n). My first attempt at this failed due to overlapping impls (the 1×1 case). I then tried a few ways of expressing the constraint "nrows == 1 or ncols == 1" using the type system but none of my attempts worked. I think expressing the constraint should be possible, maybe with something like n_rows.saturating_sub(1) * n_cols.saturating_sub(1) == 0, but typenum doesn't appear to have a type-level saturating_sub operation. Maybe @sebcrozet has an idea how to express the constraint?
• It would be nice to avoid cloning the data when the source is an owned matrix. This is blocked on rustsim/nalgebra#479.
• I haven't implemented conversions in the other direction yet (ndarray -> nalgebra). Those should be placed in the nalgebra crate.
• It might make sense to create a separate array-convert crate for inter-crate conversions instead of using features for conditional compilation. However, that would prevent the conversions from using the From trait.
• It would be good to add some more tests before merging this.
• This is a breaking change because nalgebra requires repr(transparent), which isn't available in Rust 1.27.

[bluss]

This makes nalgebra a public dependency, optional or not. Then we'll have to be careful. Nice feature for ndarray users but we need to think about release management.

[jturner314]

we need to think about release management.

It turns out that Rust 1.31 will fix this for us, because 1.31 stabilizes the rename-dependency cargo feature (see cargo docs), which allows us to depend on multiple versions of the same crate. I just pushed a commit demonstrating support for both nalgebra 0.15 and 0.16. (I don't think we actually want to support 0.15; it's a proof of concept.) It builds on Rust 1.31 (the current beta) and later.

With this approach, if nalgebra releases a breaking change, we can add support for the new version of nalgebra in a minor release of ndarray. When ndarray releases a breaking change, we can drop support for old versions of nalgebra.

[bluss]

Not bad, that's very interesting. Great to have that stable in cargo :slightly_smiling_face:

[bluss]

Is it viable for nalgebra to implement conversions to our types? Does the dependency cycle work out if one includes both crates?

[bluss]

From impls should in general not fail or panic, so we need a rationale

[jturner314]

Is it viable for nalgebra to implement conversions to our types?

Do you mean conversions from our types? Not all conversions are possible without copying the data because nalgebra owned matrices are column-major and nalgebra slices (i.e. views) allow only non-negative strides. Additionally, I'm not sure if slices that are interlaced in memory (e.g. two rows in a column-major array) are allowed. However, if nothing else, nalgebra can implement conversions from Array that copy the data only if it's not in column-major layout and otherwise take ownership of the Vec without copying.

Does the dependency cycle work out if one includes both crates?

I assume so, but I need to check this.

From impls should in general not fail or panic, so we need a rationale

The implementations panic only in very unusual cases (overflowing isize or aliasing mutable elements). @sebcrozet said that it's a bug that nalgebra currently allows aliasing in mutable views (rustsim/nalgebra#473), so once the bug is fixed, that case is no longer a concern.

So, panics should be rare. That said, maybe we should use TryFrom? Alternatively, maybe we could convince the nalgebra team to match ndarray's constraints regarding overflowing isize? nalgebra might have some of the same issues ndarray did before that was fixed.

[vadixidav]

I know this is a pretty old open PR, but I just published a crate that has conversions from nalgebra to ndarray types in it: https://crates.io/crates/nshare (currently the only conversions it supports). I just wanted to note that this functionality can be added using a third party library so this doesn't have to be constantly updated to match the current nalgebra version.

[ZuseZ4]

Great to see it available in an extra crate. This basically means we can close it here, I guess? Just to not have too many outdated pull requests open. @bluss @xd009642

[xd009642]

Yeah IMO I think it should be fine to close it and do a docs PR to mention nshare. That seems like the path of least resistance and if we want to add direct support for this in ndarray can revisit this later.

[xd009642]

@ZuseZ4 I've done the docs PR https://github.com/rust-ndarray/ndarray/pull/847

[nilgoyette]

This is a good idea only if @vadixidav updates his nshare crate often, which is not currently the case. We can see in his Cargo.toml

nalgebra = { version = "0.20.0", default-features = false, optional = true }

but nalgebra is now at version 0.23.1

[vadixidav]

This is a good idea only if @vadixidav updates his nshare crate often, which is not currently the case. We can see in his Cargo.toml

nalgebra = { version = "0.20.0", default-features = false, optional = true }

but nalgebra is now at version 0.23.1

I have not been using the crate recently, but feel free to create an issue on the repo, and I can bump the version. I would also like to add that nshare needs some work because it only supports some conversions. Between image, ndarray, and nalgebea there are several modes of conversion that aren't currently supported. I am definitely open to taking PRs on any of that as well.

[xd009642]

@nilgoyette I've seen vadixidav be pretty responsive when users have asked for a bump. Also I think I might have write access to nshare as well so if needs be I can step in as well as other members of the rust-cv org :slightly_smiling_face:

[vadixidav]

@nilgoyette I've seen vadixidav be pretty responsive when users have asked for a bump. Also I think I might have write access to nshare as well so if needs be I can step in as well as other members of the rust-cv org 🙂

Not only can other people step in, but perhaps we need to move it to be maintained by someone else that is actually making use of it. Right now we aren't using it that actively. It probably will have to be updated before a future release of the cv crate.

Just as a note, that crate was created mostly in response to a lack of interoperability between the many n-dimensional crates. If the desire is to upstream this kind of behavior, I am totally on board with that too. I just want to make sure the tools are out there. nshare is not even complete anyways, and still needs several different conversions added. It mostly only has things added as they are needed for Rust CV, but I could easily just make those PRs upstream in nalgebea or ndarray. I think the question is which one should they be in, or if they should be separate then who maintains that?

Anyways, please let me know if you want ownership over nshare. It lives in the Rust CV org, but it isn't computer vision specific, so anyone that wants to maintain it can do that.

[vadixidav]

I did just go ahead and bump the versions in nshare, and I can try and keep that up-to-date, but if anyone wants to move that to the rust-ndarray organization, it might make more sense for it to live here. Any thoughts?

[CGMossa]

Can I ask what is the status of this? I'm trying to figure out how to take an Array2<_> from ndarray and construct nalgebra::base::Matrix, and then I found this PR.

[vadixidav]

Can I ask what is the status of this? I'm trying to figure out how to take an Array2<_> from ndarray and construct nalgebra::base::Matrix, and then I found this PR.

So far the situation is the same. We are still maintaining nshare. Feel free to use that to perform conversions. Unfortunately, updates to nalgebra are quite rapid and can create breakage, so please raise an issue if it goes out of date for you.

[bluss]

Thanks for doing it too, interoperability is quite important, but figuring out how to handle the cargo deps for it is tricky IMO.