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

Compilation error with CsMat and generic Float #278

Open
relf opened this issue Mar 22, 2021 · 26 comments
Open

Compilation error with CsMat and generic Float #278

relf opened this issue Mar 22, 2021 · 26 comments
Assignees

Comments

@relf
Copy link

relf commented Mar 22, 2021

I am new to sprs library, I do not understand why the following program does not compile:

use num_traits::Float;
use sprs::CsMat;

fn test<F: Float>(val: F) -> CsMat<F> {
    let a = CsMat::new_csc(
        (3, 3),
        vec![0, 2, 4, 5],
        vec![0, 1, 0, 2, 2],
        vec![val, val, val, val, val],
    );
    let b = &a + &a;
    b
}

fn main() {
    let a = test(1.);
}

with the following dependencies

[dependencies]
sprs = "0.10"
num-traits = "0.2"

and when I try to build I get:

error[E0369]: cannot add `&CsMatBase<F, usize, Vec<usize>, Vec<usize>, Vec<F>>` to `&CsMatBase<F, usize, Vec<usize>, Vec<usize>, Vec<F>>`
  --> src\main.rs:11:16
   |
11 |     let b = &a + &a;
   |             -- ^ -- &CsMatBase<F, usize, Vec<usize>, Vec<usize>, Vec<F>>
   |             |
   |             &CsMatBase<F, usize, Vec<usize>, Vec<usize>, Vec<F>>

error: aborting due to previous error

For more information about this error, try `rustc --explain E0369`.
error: could not compile `test-sprs`

To learn more, run the command again with --verbose.

I cannot see what is missing to enable the addition. Thanks for your help.

@mulimoen
Copy link
Collaborator

mulimoen commented Mar 22, 2021

In short: F must support addition, and the error message is poor.

Longer: To support addition for arbitrary types, we must restrict F by applying the appropriate trait bounds. Which ones are a bit cryptic, but we can dig through the sprs code to find the impl for Add here: https://github.com/vbarrielle/sprs/blob/f815271c8543981d6c926fe2415ef722bda7ceaa/src/sparse/binop.rs#L20
The bound on F is given by the following line:
https://github.com/vbarrielle/sprs/blob/f815271c8543981d6c926fe2415ef722bda7ceaa/src/sparse/binop.rs#L24
You can modify test to include this bound by:

fn test<F: Float>(val: F) -> CsMat<F>
where
    F: num_traits::Zero + PartialEq + Clone + Default,
{

and your code should compile.

Edit: It should be enough to specify the additional trait bound Default.

@relf
Copy link
Author

relf commented Mar 22, 2021

Thanks for the swift reply. Actually I went down that way, but in that case I got:

error[E0308]: mismatched types
  --> src\main.rs:14:18
   |
14 |     let b = &a + &a;
   |                  ^^ expected struct `ndarray::ArrayBase`, found struct `CsMatBase`
   |
   = note: expected reference `&ndarray::ArrayBase<_, ndarray::dimension::dim::Dim<[usize; 2]>>`
              found reference `&CsMatBase<F, usize, Vec<usize>, Vec<usize>, Vec<F>>`

error[E0308]: mismatched types
  --> src\main.rs:15:5
   |
4  | fn test<F: Float>(val: F) -> CsMat<F>
   |                              -------- expected `CsMatBase<F, usize, Vec<usize>, Vec<usize>, Vec<F>>` because of return type
...
15 |     b
   |     ^ expected struct `CsMatBase`, found struct `ndarray::ArrayBase`
   |
   = note: expected struct `CsMatBase<F, usize, Vec<usize>, Vec<usize>, Vec<F>>`
              found struct `ndarray::ArrayBase<ndarray::data_repr::OwnedRepr<F>, ndarray::dimension::dim::Dim<[usize; 2]>>`

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0308`.
error: could not compile `test-sprs`

It seems to pick the addition with a ndarray! Here the modified program:

use num_traits::Float;
use sprs::CsMat;

fn test<F>(val: F) -> CsMat<F>
where
    F: Float + num_traits::Zero + PartialEq + Clone + Default,
{
    let a = CsMat::new_csc(
        (3, 3),
        vec![0, 2, 4, 5],
        vec![0, 1, 0, 2, 2],
        vec![val, val, val, val, val],
    );
    let b = &a + &a;
    b
}

fn main() {
    let a = test(1.);
}

@mulimoen
Copy link
Collaborator

Oh, I see my cargo add used 0.9 of sprs. That does not look good, I'll check again to see what is going on here

@mulimoen
Copy link
Collaborator

My next attempt involves adding this bound:

for<'r> &'r F: core::ops::Add<&'r F, Output = F>,

but this leads to a recursion error in type resolution.

@relf
Copy link
Author

relf commented Mar 22, 2021

Yep did that too! 👀 Then I opened the issue thinking I was missing something.

@mulimoen
Copy link
Collaborator

Paging @vbarrielle, this was introduced in d2f0da7. Any ideas in why type-checking would go haywire here?

@relf
Copy link
Author

relf commented Mar 22, 2021

I confirm it compiles with 0.9.3 adding only the Default trait as you mentioned. As far as I am concerned, I want to use ndarray 0.14, so...

@vbarrielle
Copy link
Collaborator

I'll need to investigate this issue. I've definitely encountered the type resolution recursion issue (see rust-lang/rust#82779) while developping d2f0da7.

This error happens because I've tried to minimize the number of clones in the binary operations, and I picked my traits to be sure that it would work on classic scalar types. However I failed to consider the case of a dependent crate using a generic scalar type, which is quite a problem...

@bytesnake
Copy link

any update here? This blocks bumping ndarray = "0.14" atm

@mulimoen
Copy link
Collaborator

As a temporary measure we could backport the ndarray bump to 0.9.

@relf
Copy link
Author

relf commented Apr 14, 2021

Yes, I think it would be great to have that backup solution in the meantime, not to fall too much behind as ndarray 0.15.1 is already out.

@mulimoen
Copy link
Collaborator

@vbarrielle I've added a new branch 0.9.4 branching off the 0.9.3 tag. This bumps ndarray for sprs and sprs-ldl.

@relf
Copy link
Author

relf commented Apr 15, 2021

@mulimoen FWIW, your 0.9.4 branch works for linfa with ndarray 0.14 (rust-ml/linfa#110).

@vbarrielle
Copy link
Collaborator

Hello, sorry for the silence, things are a bit complicated here and I don't have time to work on this right now (and probably for the next month as well).

Thanks @mulimoen for making this 0.9.4 branch. @mulimoen if you accept I'm giving you upload rights on crates.io to push that version.

@mulimoen
Copy link
Collaborator

Thanks @vbarrielle, 0.9.4 should be uploaded now.

@bytesnake
Copy link

Hello, sorry for the silence, things are a bit complicated here and I don't have time to work on this right now (and probably for the next month as well).

sure take your time, it is not super urgent but we don't want to lack too far behind ndarray

Thanks @vbarrielle, 0.9.4 should be uploaded now.

cool thanks!

@relf
Copy link
Author

relf commented Apr 16, 2021

Thanks @vbarrielle for your hard work!

@mulimoen
Copy link
Collaborator

I think the original compilation error is gone with the following extra bounds:

fn test<F>(val: F) -> CsMat<F>
where
    F: Float,
    F: Default + Send + Sync + num_traits::MulAdd<Output = F>,
{
    let a = CsMat::new_csc(
        (3, 3),
        vec![0, 2, 4, 5],
        vec![0, 1, 0, 2, 2],
        vec![val, val, val, val, val],
    );
    let b = &a * &a;
    b
}

@relf
Copy link
Author

relf commented Apr 18, 2021

Yes, it works, thanks! I am wondering how you found that list of bound spells though! 😄

I still have to try this with linfa 😅. Thanks again.

@bytesnake
Copy link

this seems to be broken again (perhaps a regression introduced in #290). The problem can be worked around by using csmat_binop(a.view(), a.view(), |x, y| x.add(*y)) instead of this https://docs.rs/sprs/0.11.0/src/sprs/sparse/binop.rs.html#63 which aborts with type recursion. Reopen?

@mulimoen
Copy link
Collaborator

mulimoen commented Jul 31, 2021

@bytesnake This does not seem related to the original issue. I've opened #292 to address this issue
edit: This is indeed the same problem

@mulimoen mulimoen reopened this Jul 31, 2021
@mulimoen
Copy link
Collaborator

I also see that my "fix" did not actually work as I replaced addition by multiplication

@StefanUlbrich
Copy link

Did anybody find a workaround? How does this recursion error occur?

@StefanUlbrich
Copy link

StefanUlbrich commented Aug 2, 2022

Would it help to create another blanket type around for the HRBT? That code does not raise the recursion error where it did before but I cannot really test it right now:

pub trait Real<T>: Num + NdFloat + Default {}
pub trait RealRef<S, T>: Add<S, Output=T> + Mul<S, Output=T> {}

impl Real<f32> for f32 {}
impl Real<f64> for f64 {}

impl RealRef<&f32,f32> for &f32 {}
impl RealRef<&f64,f64> for &f64 {}

fn test1<T, D>( y: Array<T, D>)
where
    T: Real<T>,
    for<'r> &'r T: RealRef<&'r T, T>,
    D: Dimension,
{
}

Could sprs provide a suitable blanket trait comparable to NDFloat if this worked?

Edit: It seems to work

@mulimoen
Copy link
Collaborator

mulimoen commented Aug 5, 2022

@StefanUlbrich How should this fix be integrated into sprs?

@StefanUlbrich
Copy link

StefanUlbrich commented Aug 5, 2022

It should suffice to include the two traits with implementations for the relevant types (does integer or complex make sense?). That way, sprs can be used in libraries with generic types.
They could be called SPRSfloat and SPRSfloatRef (unless someone more experienced with rust knows how this can be achieved with a single one).
In addition, I combined only the traits that I needed for the csaps-rs package. More might make sense

edit: do you have a recommendation for a location, @mulimoen ? I’d be happy to make a PR

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

No branches or pull requests

5 participants