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

Add "forwards" of the approx methods .abs_diff_eq and .relative_eq #946

Merged
merged 1 commit into from
Mar 18, 2021

Conversation

bluss
Copy link
Member

@bluss bluss commented Mar 17, 2021

This is a nod to the all_close method that was removed. The replacement
methods still require the "approx" feature gate, but we forward them as
inherent methods on the array, to make them easier to call (no trait
import needed).

Closes #763

Copy link
Member

@jturner314 jturner314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good idea. I just had a small comment regarding the type parameters of the methods.

There are a few places in doc tests (e.g. for logspace and mapv_inplace) which use approx, which could optionally be replaced with these methods, if desired.

/// approximate equality of two arrays.
///
/// **Requires crate feature `"approx"`**
pub fn abs_diff_eq<B, S2>(&self, other: &ArrayBase<S2, D>, epsilon: A::Epsilon) -> bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, this would be better with a single type parameter for simplicity:

pub fn abs_diff_eq<S2>(&self, other: &ArrayBase<S2, D>, epsilon: A::Epsilon) -> bool
where
    A: AbsDiffEq<S2::Elem>,
    A::Epsilon: Clone,
    S2: Data,
{}

/// apart; and the absolute difference otherwise.
///
/// **Requires crate feature `"approx"`**
pub fn relative_eq<B, S2>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

This is a nod to the all_close method that was removed. The replacement
methods still require the "approx" feature gate, but we forward them as
inherent methods on the array, to make them easier to call (no trait
import needed).
@bluss
Copy link
Member Author

bluss commented Mar 18, 2021

Thanks for the input 🙂

@bluss bluss merged commit 141facd into master Mar 18, 2021
@bluss bluss deleted the array-approx branch March 18, 2021 23:19
@bluss bluss added this to the 0.15.0 milestone Mar 18, 2021
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

Successfully merging this pull request may close these issues.

Should ArrayBase::all_close be un-deprecated?
2 participants