ndarray icon indicating copy to clipboard operation
ndarray copied to clipboard

Documentation clarification for approx feature

Open rcarson3 opened this issue 5 years ago • 8 comments
trafficstars

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.

rcarson3 avatar Mar 16 '20 01:03 rcarson3

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 avatar Apr 12 '20 11:04 bluss

@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.

rcarson3 avatar Apr 12 '20 21:04 rcarson3

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 avatar Apr 12 '20 22:04 bluss

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.

bluss avatar Apr 18 '20 09:04 bluss

@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.

rcarson3 avatar Apr 18 '20 16:04 rcarson3

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 avatar Jul 23 '21 02:07 multimeric

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" ] }

multimeric avatar Jul 23 '21 02:07 multimeric

@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

rcarson3 avatar Jul 23 '21 02:07 rcarson3