Clarify which (arg)min/max index/value is returned #32
Conversation
For performance, it's beneficial to avoid guaranteeing a particular iteration order. For example, the current implementation of `min` may return any of the minima because the iteration order of `ArrayBase.fold()` is unspecified. The current implementation of `argmin` does always return the first minimum (in logical order) since `ArrayBase.indexed_iter()` always iterates in logical order, but we may want to optimize the iteration order in the future.
jturner314
force-pushed
the
min-max-docs
branch
from
8691e32 to
3bd9c1a
Compare
tests/quantile.rs
Outdated
| @@ -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))); | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 ❤️
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
Yes, this is better than what I wrote originally. I've added another commit using quickcheck. Thanks!
There was a problem hiding this comment.
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.
|
I agree it makes sense to relax the guarantees we provide. |
See discussion at #32 (comment)
|
I'll merge 👍 |
For performance, it's beneficial to avoid guaranteeing a particular iteration order. For example, the current implementation of
minmay return any of the minima because the iteration order ofArrayBase.fold()is unspecified. The current implementation ofargmindoes always return the first minimum (in logical order) sinceArrayBase.indexed_iter()always iterates in logical order, but we may want to optimize the iteration order in the future.