Skip to content

Clarify which (arg)min/max index/value is returned #32

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 14, 2019
Merged

Clarify which (arg)min/max index/value is returned #32

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3bd9c1a
Clarify which (arg)min/max index/value is returned
jturner314 Mar 11, 2019
dbcebde
Replace some tests with argmin/max_matches_min/max
jturner314 Mar 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 26 additions & 2 deletions src/quantile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,14 +182,18 @@ where
S: Data<Elem = A>,
D: Dimension,
{
/// Finds the first index of the minimum value of the array.
/// Finds the index of the minimum value of the array.
///
/// Returns `None` if any of the pairwise orderings tested by the function
/// are undefined. (For example, this occurs if there are any
/// floating-point NaN values in the array.)
///
/// Returns `None` if the array is empty.
///
/// Even if there are multiple (equal) elements that are minima, only one
/// index is returned. (Which one is returned is unspecified and may depend
/// on the memory layout of the array.)
///
/// # Example
///
/// ```
Expand All @@ -214,12 +218,20 @@ where
/// floating-point NaN values in the array.)
///
/// Additionally, returns `None` if the array is empty.
///
/// Even if there are multiple (equal) elements that are minima, only one
/// is returned. (Which one is returned is unspecified and may depend on
/// the memory layout of the array.)
fn min(&self) -> Option<&A>
where
A: PartialOrd;

/// Finds the elementwise minimum of the array, skipping NaN values.
///
/// Even if there are multiple (equal) elements that are minima, only one
/// is returned. (Which one is returned is unspecified and may depend on
/// the memory layout of the array.)
///
/// **Warning** This method will return a NaN value if none of the values
/// in the array are non-NaN values. Note that the NaN value might not be
/// in the array.
Expand All @@ -228,14 +240,18 @@ where
A: MaybeNan,
A::NotNan: Ord;

/// Finds the first index of the maximum value of the array.
/// Finds the index of the maximum value of the array.
///
/// Returns `None` if any of the pairwise orderings tested by the function
/// are undefined. (For example, this occurs if there are any
/// floating-point NaN values in the array.)
///
/// Returns `None` if the array is empty.
///
/// Even if there are multiple (equal) elements that are maxima, only one
/// index is returned. (Which one is returned is unspecified and may depend
/// on the memory layout of the array.)
///
/// # Example
///
/// ```
Expand All @@ -260,12 +276,20 @@ where
/// floating-point NaN values in the array.)
///
/// Additionally, returns `None` if the array is empty.
///
/// Even if there are multiple (equal) elements that are maxima, only one
/// is returned. (Which one is returned is unspecified and may depend on
/// the memory layout of the array.)
fn max(&self) -> Option<&A>
where
A: PartialOrd;

/// Finds the elementwise maximum of the array, skipping NaN values.
///
/// Even if there are multiple (equal) elements that are maxima, only one
/// is returned. (Which one is returned is unspecified and may depend on
/// the memory layout of the array.)
///
/// **Warning** This method will return a NaN value if none of the values
/// in the array are non-NaN values. Note that the NaN value might not be
/// in the array.
Expand Down
6 changes: 4 additions & 2 deletions tests/quantile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ fn test_argmin() {
assert_eq!(a.argmin(), None);

let a = array![[1, 0, 3], [2, 0, 6]];
assert_eq!(a.argmin(), Some((0, 1)));
let argmin = a.argmin();
assert!(argmin == Some((0, 1)) || argmin == Some((1, 1)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: should we delete this?

I'm trying to get my head around the reason this test exists. I can't help feeling it could be about characterizing the behavior of the algorithm given two equal minima. If that's the case and we don't want to specify that tightly then maybe these tests (this and the one for argmax) aren't required anymore?

Maybe they offer some comfort that it doesn't blow up where there are multiple minima, but could it even?

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense to have it as good documentation: it makes it clear that we are not committing on which of the various minima we are going to return.

Copy link
Member

Choose a reason for hiding this comment

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

To offer more context, in my opinion a test suite serves multiple purposes:

  • regression testing, to check that we are not violating the contract with our users without noticing it;
  • correctness, to iron out and make sure that stuff works;
  • docs, to offer an example of how things are supposed to be used and what guarantees you should expect from them (tightly coupled to regression testing).

I personally skim through the test suite of a project before even starting looking at the piece of code I am interested to (unless it's trivial, of course).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. Thought about it overnight. This is how I've seen this kind of thing done in the past:

    let a = array![[1, -17, 3], [2, -17, 6]];
    assert!(a[a.argmin().unwrap()] == -17);

Essentially we use the return from argmin/max and assert against the value lookup instead. It asserts the truth we really care about without committing us to implementation details.

I agree the OR-ing approach is good to show our intent. But it does have a small problem: the test will always pass as long as at least one of the two operands is the actual value returned, rendering the second one effective "dead". Even if we change the implementation in future, still only one of the two operands is doing anything. In fact, we could specify any of the other valid index-pairs for the "dead" one and the test would still pass.

This is the kind of thing that can lead to tests that accidentally never fail. They usually get like that after a few iterations of refactoring and different contributors dropping in (with good intent).

Honestly, I don't think this there is a big risk in this case. So it's more of a handwriting/good-practice suggestion.

Because I'm new here I'm going to point out that:

  • This is just a suggestion so I won't be offended and go away if you decide not to change it ❤️

Copy link
Member

Choose a reason for hiding this comment

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

I understand where you are coming from and I definitely agree with what you are saying: tests should only test the intended functionality, not the implementation.
We could actually augment

assert!(a[a.argmin().unwrap()] == -17);

and transform it into a property test using quickcheck:

assert!(a[a.argmin()] == a.min());

This is even more robust (it uses random inputs, so it should get into all sort of edge cases) and I think it does convey the intent.
My issue was more about not deleting it then refactoring it: suggestions are more than welcome and yours have proved to be insightful more than once already :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is better than what I wrote originally. I've added another commit using quickcheck. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Fwiw, I've implemented the quickcheck test only for 1-D arrays because that was the most straightforward option. It would be nice to test with higher-dimensional arrays too. I've created rust-ndarray/ndarray#596 to help with this.


let a: Array2<i32> = array![[], []];
assert_eq!(a.argmin(), None);
Expand Down Expand Up @@ -65,7 +66,8 @@ fn test_argmax() {
assert_eq!(a.argmax(), None);

let a = array![[1, 5, 6], [2, 0, 6]];
assert_eq!(a.argmax(), Some((0, 2)));
let argmax = a.argmax();
assert!(argmax == Some((0, 2)) || argmax == Some((1, 2)));

let a: Array2<i32> = array![[], []];
assert_eq!(a.argmax(), None);
Expand Down