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

BUG: np.equal.reduce no longer works with int and float #20929

Closed
brisvag opened this issue Jan 28, 2022 · 11 comments · Fixed by #21981
Closed

BUG: np.equal.reduce no longer works with int and float #20929

brisvag opened this issue Jan 28, 2022 · 11 comments · Fixed by #21981
Labels
05 - Testing 57 - Close? Issues which may be closable unless discussion continued sprintable Issue fits the time-frame and setting of a sprint

Comments

@brisvag
Copy link

brisvag commented Jan 28, 2022

Describe the issue:

In 1.22.1 the below code fails. However, it used to work in 1.21.5. It still works with object and bool dtypes.

Reproduce the code example:

np.equal.reduce([0, 0])

Error message:

TypeError: No loop matching the specified signature and casting was found for ufunc equal

NumPy/Python version information:

1.22.1 3.10.2 (main, Jan 15 2022, 19:56:27) [GCC 11.1.0]

@soma2000-lang
Copy link
Contributor

@brisvag I am working on this

@seberg
Copy link
Member

seberg commented Jan 28, 2022

Hmmmm... I am wondering if we shouldn't just call it a bug-fix. I am serious, I do not have an intuition for the following:

np.equal.reduce([1, 2]) == True

The result is always the same (except for pure object), as np.equal.reduce([0, 0], dtype=bool), which still works and is at least somewhat clear about the fact that all that happens is that the input is an unsafe cast to bool. (IMO, we really should force you to add casting="unsafe" as well there, but that isn't currently supported.)

@brisvag
Copy link
Author

brisvag commented Jan 28, 2022

@seberg I see, I was not aware of the dtype argument. That seems to work around my issue with ints.

However, I'm a bit confused. Just to make sure I follow: the dtype argument in a reduce call is the dtype of the intermediate result, according to the docstring. Why is this affecting my result? Is it wrong to expect this to work without passing any additional argument? I think I misudnerstand what reduce does :P

a = np.array(['x', 'y'])
b = np.stack([a, a, a], axis=1)

np.equal.reduce(b, axis=1)  # (2,) array of bools, all True

@seberg
Copy link
Member

seberg commented Jan 28, 2022

I don't think you are misunderstanding what reduce does, but I do think you are not realizing what it ends up doing :p. A reduce operation is defined by the following (roughly, how the start is handled depends and I think it makes the situation worse for equal):

res = bool(arr[0])  # res is *always* boolean (except for `object`)
for i in range(1, len(arr)):
    res = res == bool(arr[i])

Now, the type of res is fixed to bool (except for object where it can be also object). The fact that it is:

    res = res == bool(arr[i])

and not:

    res = bool(res == arr[i])

might be surprising. But even then, what do you expect the intermediate result to be? Do you expect the same as (arr[1:] == arr[:-1]).all()? That would not be a reduce operation.

@brisvag
Copy link
Author

brisvag commented Jan 28, 2022

Now, the type of res is fixed to bool (except for object where it can be also object). The fact that it is:

    res = res == bool(arr[i])

and not:

    res = bool(res == arr[i])

might be surprising. But even then, what do you expect the intermediate result to be? Do you expect the same as (arr[1:] == arr[:-1]).all()? That would not be a reduce operation.

This is very surprising indeed! So how is something like np.less.reduce even working? Shoudn't that then be:

res = res < bool(arr[i])

Which would certainly fail? Or is casting to bool first a quirk of equal?

Do you expect the same as (arr[1:] == arr[:-1]).all()? That would not be a reduce operation.

I think I do expect that... which probably means reduce is just not the thing I should be using...

@seberg
Copy link
Member

seberg commented Jan 28, 2022

I think I do expect that... which probably means reduce is just not the thing I should be using...

Yes, well the new NumPy also rejects the thing for less ;). And it is why I am saying that we should likely just declare it a bug-fix, you are better off with the error message informing you that this is weird, then getting a result and expecting a different result :).

Note that the last one is really the same as (arr == arr[0]).all() (assuming you have a 1-D array at least).

@brisvag
Copy link
Author

brisvag commented Jan 28, 2022

I see, makes sense! Thanks for the explanation, things are clearer now, and it does indeed look like I was using a bug. To clarify, my actual use case was slightly more complex:

a = ...  # (n, 2) array
np.equal.reduce(a, axis=1)

But since it's just (n, 2) I suppose I should simply do a[:, 0] == a[:, 1] instead.

@seberg seberg added 57 - Close? Issues which may be closable unless discussion continued 05 - Testing and removed 00 - Bug labels Jan 28, 2022
@seberg
Copy link
Member

seberg commented Jan 28, 2022

If we consider this a bug fix (or even if not), it might be nice to add a test case for this, though. Even if someone changes behaviour in the future, it is better to have a test that notifies you that there was a change.

I am not sure we have one. @soma2000-lang just in case you are interested in that.

@seberg seberg added the sprintable Issue fits the time-frame and setting of a sprint label Apr 29, 2022
mocquin added a commit to mocquin/physipy that referenced this issue May 7, 2022
…tests

tests were previously passing but updating numpy made them failed. See numpy/numpy#20929
For now all we want is the same behaviour as with standard np.ndarray
@Saransh-cpp
Copy link
Contributor

Hi, @seberg, @brisvag! New here! Just went through the discussion above, and I am assuming that this would require checking if np.equal.reduce([0, 0]) raises a TypeError?

I went through some similar looking tests and found this -

def test_reduce(self):
# Ticket #40
assert_almost_equal(np.add.reduce([1., .5], dtype=None), 1.5)

Should I add a test in this function? Or could you please point out where this test should be added? Thanks!

@seberg
Copy link
Member

seberg commented Jul 13, 2022

Thanks for having a look. I try to avoid adding regresssion tests, since they are even less structured than the rest of our tests. I think this class would be a good place:

class TestComparisons:

@Saransh-cpp
Copy link
Contributor

Thank you for the quick reply, @seberg!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
05 - Testing 57 - Close? Issues which may be closable unless discussion continued sprintable Issue fits the time-frame and setting of a sprint
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants