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

Allow ops on arrays with elems of different types #588

Merged
merged 2 commits into from
May 5, 2019

Conversation

jturner314
Copy link
Member

This generalizes the right operand element type. The implementations of operations for &'a ArrayBase<S, D> are still unnecessarily restrictive in the Output element type, but the implementations are now more general than they were before this commit.

As far as I can tell, this change is backwards compatible except for possibly causing ambiguities in type inference that weren't present before.

The implementations of operations for `&'a ArrayBase<S, D>` are still
unnecessarily restrictive in the `Output` element type, but the
implementations are now more general than they were before this
commit.
@LukeMathWalker
Copy link
Member

It looks good to me, but it would be nice to have a way to check if it introduces type inference ambiguities that might cause a project using ndarray to fail compilation.
I think it would desirable, generally speaking, for us to have a cohort of projects to keep an eye on in terms of usage patterns on which to assess the impact of changes (similarly to what crater does for Rust, although on a much bigger scale).

@jturner314
Copy link
Member Author

I think it would desirable, generally speaking, for us to have a cohort of projects to keep an eye on in terms of usage patterns on which to assess the impact of changes (similarly to what crater does for Rust, although on a much bigger scale).

Yeah, that would be nice. It might be possible to do this using crates on crates.io that depend on the latest version of ndarray.

It looks good to me, but it would be nice to have a way to check if it introduces type inference ambiguities that might cause a project using ndarray to fail compilation.

I have a (non-public) project with ~1.3e4 lines of code that uses ndarray pretty heavily. I just ran cargo check with this PR. There are some cases that work in ndarray 0.12.1 where type inference is no longer sufficient with this PR.

This is one (simplified) example:

extern crate ndarray;

use ndarray::prelude::*;

fn main() {
    let n = 5;
    let a = Array2::<f64>::zeros((n, n));
    let b = a + Array2::eye(n);
    println!("{}", b);
}

(The compiler can no longer infer the element type for Array2::eye.)

Here's another similar example:

extern crate ndarray;

use ndarray::prelude::*;

fn main() {
    let n = 5;
    let a = Array1::<f64>::zeros(n);
    let b = Array2::zeros((n, n)) + a;
    println!("{}", b);
}

(The compiler can no longer infer the element type for Array2::zeros.)

I've added the "breaking-change" label. I don't see any way around this since Rust doesn't let us specify a default type for impls. The nice thing is that any breakage is easy to identify and fix. I really think allowing different element types is the right way to go.

If everything looks good, let's go ahead and merge this. (We've already started merged some breaking changes for the 0.13 release.)

@LukeMathWalker
Copy link
Member

I had somehow lost track of this conversation.
Shall we go ahead and merge it?

@jturner314 jturner314 merged commit 7733b7c into rust-ndarray:master May 5, 2019
@jturner314 jturner314 deleted the generalize-op-types branch May 5, 2019 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants