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] indexing with Booleans leads to crash #672

Closed
v923z opened this issue Jul 9, 2024 · 29 comments
Closed

[BUG] indexing with Booleans leads to crash #672

v923z opened this issue Jul 9, 2024 · 29 comments
Labels
bug Something isn't working critical critical bug, requires speedy fix

Comments

@v923z
Copy link
Owner

v923z commented Jul 9, 2024

As mentioned in #671, the following code leads to a crash:

filtered_boxes = np_boxes[np_boxes[:, 4] > 0.0]

@kwagyeman

@v923z v923z added the bug Something isn't working label Jul 9, 2024
@v923z
Copy link
Owner Author

v923z commented Jul 15, 2024

Wouldn't it be better to use https://numpy.org/doc/stable/reference/generated/numpy.select.html for this? My reasoning is that first, that would lead to a cleaner implementation, second, it would be easier to customise the firmware. Excluding a function/method is more meaningful than excluding a sub-feature of a feature.

@kwagyeman
Copy link

Sounds fine. Just need a way to do it.

@kwagyeman
Copy link

kwagyeman commented Jul 18, 2024

Question, does where() work?

def yolov5_vectorized(model, input, output):
    out = output[0]
    ib, ih, iw, ic = model.input_shape[0]
    ob, ow, oc = model.output_shape[0]
    if ob != 1:
        raise ValueError("Expected model output batch to be 1!")
    if oc < 6:
        raise ValueError("Expected model output channels to be >= 6")

    # Extract relevant output slices
    scores = out[0, :, 4]
    coords = out[0, :, :4]

    # Filter indices where score > 0.5
    valid_indices = np.where(scores > 0.5)[0]

    # Compute bounding box coordinates
    cx = coords[valid_indices, 0]
    cy = coords[valid_indices, 1]
    cw = coords[valid_indices, 2] * 0.5
    ch = coords[valid_indices, 3] * 0.5

    xmin = (cx - cw) * iw
    ymin = (cy - ch) * ih
    xmax = (cx + cw) * iw
    ymax = (cy + ch) * ih

    # Compute label index
    labels = out[0, valid_indices, 5:]
    label_index = np.argmax(labels, axis=1)

    # Create bounding boxes
    nms = NMS(iw, ih, input[0].roi)
    for i in range(len(valid_indices)):
        nms.add_bounding_box(xmin[i], ymin[i], xmax[i], ymax[i], scores[valid_indices[i]], label_index[i])

    boxes = nms.get_bounding_boxes()
    return boxes

I could use it to accomplish my goal of being able to select indices. ChatGPT wrote the code above, though, and it might have made a mistake regarding how to select columns.

@v923z
Copy link
Owner Author

v923z commented Jul 18, 2024

Yes, where should work, if you enabled it in ulab.h.

@kwagyeman
Copy link

kwagyeman commented Jul 26, 2024

Having issues with this stuff still:

a = np.array(range(36)).reshape((6, 6))

i = np.nonzero(np.asarray(a[:, 4] > 15))

print(a[i])

Doesn't work. Not sure why. I get a IndexError: indices must be integers, slices, or Boolean lists.

And then this says:

a = np.array(range(36)).reshape((6, 6))

i = np.where(a[:, 4] > 15, 1, 0)

print(i)

NotImplementedError: operation is implemented for 1D Boolean arrays only which it mentions in the docs. Could this restriction be removed? Need this to process tensor outputs.

It's not clear how to move forward without writing a for loop. Trying to stay vectorized.

@v923z
Copy link
Owner Author

v923z commented Jul 26, 2024

NotImplementedError: operation is implemented for 1D Boolean arrays only which it mentions in the docs. Could this restriction be removed? Need this to process tensor outputs.

It's not trivial (and this is why I didn't implement it in the first place), but I'll try to find a way.

@kwagyeman
Copy link

@v923z - Any updates on this? Happy to pay for this to get done sooner.

@v923z
Copy link
Owner Author

v923z commented Oct 2, 2024

The last 2-3 months were a bit hectic for me, but I'll try to devote some time to it. Sorry for the delay!

@v923z
Copy link
Owner Author

v923z commented Oct 2, 2024

I'm wondering, whether we're trying to fix something that's already correct:

a = np.array(range(36)).reshape((6, 6))

i = np.where(a[:, 4] > 15, 1, 0)

print(i)

NotImplementedError: operation is implemented for 1D Boolean arrays only which it mentions in the docs. Could this restriction be removed? Need this to process tensor outputs.

It's not clear how to move forward without writing a for loop. Trying to stay vectorized.

Here is my output:

>>> import ulab
>>> ulab.__version__
'6.5.4-2D-c'

>>> a = np.arange(36).reshape((6,6))
>>> a
array([[0, 1, 2, 3, 4, 5],
       [6, 7, 8, 9, 10, 11],
       [12, 13, 14, 15, 16, 17],
       [18, 19, 20, 21, 22, 23],
       [24, 25, 26, 27, 28, 29],
       [30, 31, 32, 33, 34, 35]], dtype=int16)
>>> a[:, 4]
array([4, 10, 16, 22, 28, 34], dtype=int16)
>>> a[:, 4] > 15
array([False, False, True, True, True, True], dtype=bool)
>>> np.where(a[:, 4] > 15, 1, 0)
array([0, 0, 1, 1, 1, 1], dtype=uint8)

Is this not what you need? I've just looked at the implementation of where, and that shouldn't throw the error that you mention.

In fact, that particular exception is raised at one location only, when you try to get a slice via a higher-dimensional tensor:

if((nindex->ndim > 1) || (nindex->boolean == false)) {
mp_raise_NotImplementedError(MP_ERROR_TEXT("operation is implemented for 1D Boolean arrays only"));
}

@kwagyeman
Copy link

kwagyeman commented Oct 2, 2024

Looks like it's working now.

a = np.array(range(36)).reshape((6, 6))

i = np.nonzero(np.asarray(a[:, 4] > 15))

print(a[i])

Doesn't work still.

@kwagyeman
Copy link

Looks like it fails on the a[i] part:

from ulab import numpy as np

a = np.array(range(36)).reshape((6, 6))

t = np.asarray(a[:, 4])
print(t)
t = t > 15
print(t)
i = np.nonzero(t)
print(i)

i = np.nonzero(np.asarray(a[:, 4] > 15))

print(a[i])

@v923z
Copy link
Owner Author

v923z commented Oct 2, 2024

Oh, I see. The problem is actually with i:

>>> i = np.nonzero(np.asarray(a[:, 4] > 15))
>>> i
(array([2, 3, 4, 5], dtype=uint16),)

As a workaround, could you try with i[0]? I'll try to figure out, why nonzero returns a tuple.

@kwagyeman
Copy link

kwagyeman commented Oct 2, 2024

? Not sure what you mean, I'm trying to slice into the array a using i to extract the matching rows.

@v923z
Copy link
Owner Author

v923z commented Oct 2, 2024

Yes, I get that, but i is actually a tuple, so that's why the slicing doesn't work. That's implemented for 1D arrays only.

>>> i
(array([2, 3, 4, 5], dtype=uint16),)

@kwagyeman
Copy link

kwagyeman commented Oct 2, 2024

Yeah, this is the operation I'd like to have so that I can use lab to vectorize non-max-suppression code. Per the post above...

@v923z
Copy link
Owner Author

v923z commented Oct 2, 2024

I think there might be two issues here: one is with i. np.asarray(a[:, 4] > 15) is clearly a 1D array

>>> np.asarray(a[:, 4] > 15)
array([False, False, True, True, True, True], dtype=bool)

The problem occurs, when this is passed to nonzero.

Then the second issue is that you want to use 2D Booleans for indexing/slicing.

@v923z
Copy link
Owner Author

v923z commented Oct 2, 2024

As for nonzero, the method works in the same way in numpy:

>>> from numpy import *
>>> x = arange(5)
>>> x
array([0, 1, 2, 3, 4])
>>> nonzero(x)
(array([1, 2, 3, 4]),)

@v923z
Copy link
Owner Author

v923z commented Oct 2, 2024

It seems to me that numpy simply ignores the second, empty, member of the tuple:

>>> a = arange(36).reshape((6, 6))
>>> a
array([[ 0,  1,  2,  3,  4,  5],
       [ 6,  7,  8,  9, 10, 11],
       [12, 13, 14, 15, 16, 17],
       [18, 19, 20, 21, 22, 23],
       [24, 25, 26, 27, 28, 29],
       [30, 31, 32, 33, 34, 35]])
>>> i = nonzero(asarray(a[:, 4] > 15))
>>> i
(array([2, 3, 4, 5]),)
>>> a[i]
array([[12, 13, 14, 15, 16, 17],
       [18, 19, 20, 21, 22, 23],
       [24, 25, 26, 27, 28, 29],
       [30, 31, 32, 33, 34, 35]])

So, if we caught that here

if((nindex->ndim > 1) || (nindex->boolean == false)) {
mp_raise_NotImplementedError(MP_ERROR_TEXT("operation is implemented for 1D Boolean arrays only"));
}
then we'd be done, right?

@kwagyeman
Copy link

kwagyeman commented Oct 2, 2024

Not sure if I'm following. The trace above is what I want to happen. However, a[i] doesn't work like above. You get IndexError: indices must be integers, slices, or Boolean lists.

EDIT: So, you are saying all you need to do is make a[i] support a tuple and grab the first element of it?

@v923z
Copy link
Owner Author

v923z commented Oct 2, 2024

EDIT: So, you are saying all you need to do is make a[i] support a tuple and grab the first element of it?

numpy works, because, while i is the same as in ulab, they simply drop the second element in the tuple. So, if we catch that particular case in the code, then the behaviour would be same on both platforms.

In the interim, you could simply use i[0]. That should work everywhere. But I'll fix this tomorrow.

@v923z v923z added the critical critical bug, requires speedy fix label Oct 2, 2024
@kwagyeman
Copy link

Great!

@v923z
Copy link
Owner Author

v923z commented Oct 3, 2024

I'm trying to understand what exactly you need, and it seems to me that #661 could do, so I'm wondering, whether I should clean that one up, and then we would kill two birds with one stone.

@kwagyeman
Copy link

I just need to be able to do your Numpy example above. To select rows from a 2D array using a 1D array of row indices.

@v923z
Copy link
Owner Author

v923z commented Oct 5, 2024

Can you check out https://github.com/v923z/micropython-ulab/tree/take and see if it works for you?

@kwagyeman
Copy link

Yeah, that should work. I'll have a list of row indexes. You may also wish to implement take_along_axis since it's just a wrapper around take.

@v923z
Copy link
Owner Author

v923z commented Oct 6, 2024

take_along_axis also allows for broadcasting, so it's a bit more than a wrapper.

@kwagyeman
Copy link

Okay, take is sufficient.

@v923z
Copy link
Owner Author

v923z commented Oct 7, 2024

OK, thanks for the feedback! I'll write up the documentation and merge the code.

@v923z v923z mentioned this issue Oct 7, 2024
@v923z
Copy link
Owner Author

v923z commented Oct 9, 2024

Fixed through #688.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working critical critical bug, requires speedy fix
Projects
None yet
Development

No branches or pull requests

3 participants
@v923z @kwagyeman and others