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

Documentation clarification for approx feature #789

Closed
rcarson3 opened this issue Mar 16, 2020 · 8 comments · Fixed by #1289
Closed

Documentation clarification for approx feature #789

rcarson3 opened this issue Mar 16, 2020 · 8 comments · Fixed by #1289

Comments

@rcarson3
Copy link
Contributor

I finally got around to updating some of my crates from ndarray v0.11.* to ndarray v0.13. I noticed that allclose was deprecated in favor of abs_diff_eq. However, I noticed in order to take advantage of it you have to make use of the approx crate within your own and then use one of their exposed macros. I'm not really a fan of this. I'd rather see the traits within approx_array module exposed to users if they have the approx feature added and available within the prelude behind a [cfg(feature = "approx")] flag. The numpy array doc also makes it seem like the abs_diff_eq is available to outside crates but as far as I can tell that's not the case.

@bluss
Copy link
Member

bluss commented Apr 12, 2020

Adding to the prelude conditionally has its problems, it could be seen as unintentionally breaking some users, due to being a non-additive change (?) it's a borderline question. Either way you can use the method from the approx traits, and don't need the macros, even if it's a bit cumbersome. Just need to use the relevant trait to use the abs_diff_eq method.

We need a featureful approximate comparison and while it's not perfect, it's good to use a separate crate to access an implementation of that.

@bluss bluss added the question label Apr 12, 2020
@rcarson3
Copy link
Contributor Author

@bluss I should have clarified in the first post. I wasn't proposing having it exposed in the prelude but instead allowing someone to do something like use ndarray::approx_aaray to access those internal functions and traits. Also, I'm with using a separate crate for the approximate comparison behind the scenes in ndarray.

A lot of this was motivated when I was updating my code from ndarray v0.11 to ndarray v0.13 (should have updated this sooner but life got in the way...) and got the following warning:

warning: use of deprecated item 'ndarray::numeric::impl_numeric::<impl ndarray::ArrayBase<S, D>>::all_close': Use `abs_diff_eq` - it requires the `approx` crate feature
  --> tests/ori_conv_tests.rs:27:30
   |
27 |     let comp = rod_comp_ori.all_close(&rod_comp_ori2, 1e-14);
   |                              ^^^^^^^^^
   |
   = note: `#[warn(deprecated)]` on by default

Based on this warning I would have expected the following to work as an update:

error[E0599]: no method named `abs_diff_eq` found for struct `ndarray::ArrayBase<ndarray::OwnedRepr<f64>, ndarray::Dim<[usize; 2]>>` in the current scope
  --> tests/ori_conv_tests.rs:27:30
   |
27 |     let _comp = rod_comp_ori.abs_diff_eq(&rod_comp_ori2, 1e-14);
   |                              ^^^^^^^^^^^ method not found in `ndarray::ArrayBase<ndarray::OwnedRepr<f64>, ndarray::Dim<[usize; 2]>>`
   |
   = help: items from traits can only be used if the trait is in scope
help: the following trait is implemented but not in scope; perhaps add a `use` for it:
   |
5  | use approx::abs_diff_eq::AbsDiffEq;
   |

mod abs_diff_eq; is private so cargo's suggestion doesn't work. The only thing I've found to work is doing the following:

assert_abs_diff_eq!(rod_comp_ori, rod_comp_ori2, epsilon = 1e-14);

I hope this kinda helps motivate what I was trying to get at with the original post. I'd say one alternative to making array_approx public would maybe having some additional documentation added related to the approx feature.

@bluss
Copy link
Member

bluss commented Apr 12, 2020

Rustc's suggestion is right in principle, just uses the wrong path. That's unfortunate.

Looks like just the documentation is missing then, an example of how to use the trait and the methods.

@bluss bluss changed the title approx_array is private to outside crates Documentation clarification for approx feature Apr 12, 2020
@bluss
Copy link
Member

bluss commented Apr 18, 2020

Sorry for not addressing your suggestion directly - ndarray doesn't contain any internal functions and traits for this functionality. What's already inside ndarray::array_approx (private module) are implementations of the approx traits. Those traits are public, so the implementations are public too, and there is nothing more to expose.

@rcarson3
Copy link
Contributor Author

@bluss that's fine. I really haven't had any time to look at this until just now. I did finally figure out what rustc meant from it's suggestion. As an external user, I'd say that the deprecation message for all_close could maybe updated to make it obvious that you have to include use approx::AbsDiffEq as well to make use of abs_diff_eq. Then an example for how to use these new approx traits could be helpful.

@multimeric
Copy link

multimeric commented Jul 23, 2021

Sorry, can either of you clarify how to properly write the example above?

error[E0599]: no method named `abs_diff_eq` found for struct `ndarray::ArrayBase<ndarray::OwnedRepr<f64>, ndarray::Dim<[usize; 2]>>` in the current scope
  --> tests/ori_conv_tests.rs:27:30
   |
27 |     let _comp = rod_comp_ori.abs_diff_eq(&rod_comp_ori2, 1e-14);
   |                              ^^^^^^^^^^^ method not found in `ndarray::ArrayBase<ndarray::OwnedRepr<f64>, ndarray::Dim<[usize; 2]>>`
   |
   = help: items from traits can only be used if the trait is in scope
help: the following trait is implemented but not in scope; perhaps add a `use` for it:
   |
5  | use approx::abs_diff_eq::AbsDiffEq;
   |

I can't for the life of me work it out, and as has been said, the compiler is suggesting I import a private approx module.

@multimeric
Copy link

Okay it seems that in order to use the approx methods you need to:

  • Depend on ndarray with the approx feature
  • Depend on approx
  • Make sure the direct dependency for approx is the same version as ndarray is using!! (RFC 1977 cannot come any faster)
  • use approx::AbsDiffEq;
  • Now you can a.abs_diff_eq(&b, 0.01);

As a full example:

use approx::AbsDiffEq;
use ndarray::*;

fn main() {
    let a = array![0.1, 0.2, 0.3];
    let b = array![0.099, 0.199, 0.299];
    let cmp = a.abs_diff_eq(&b, 0.01);
    println!("{}", cmp);
}
[package]
name = "foo"
version = "0.1.0"
edition = "2018"

[dependencies]
approx = "0.3.2"
ndarray = { version = "0.13.1", features = [ "approx" ] }

@rcarson3
Copy link
Contributor Author

@multimeric if it helps any here's my conversion from ndarray v0.11 to v0.13 in one of my crates: test changes and cargo changes. Here's the relevant changes I had to make going to v0.15.* test changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants