-
Notifications
You must be signed in to change notification settings - Fork 118
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
[FEATURE REQUEST] implement numpy.take #661
Comments
Code added in https://github.com/v923z/micropython-ulab/tree/take |
@v923z sorry to have dropped off. I will pick this up for testing. |
Hi @v923z just built this on windows using Ubuntu and it seems to have produced the firmware as expected. Running the following snippet however:
...produces None in the console. Seems like I can use the 'out' kwarg to get a result, so I tried that too:
...produces array([0], dtype=uint8) where I would expect the value array([2], dtype=uint8). Changing the 'indices' array to take more values does indeed give me more values in the out array, however they always appear to be 0. I'm no C++ dev by a long shot, but I can see the only return statement in create_take returns mp_const_none, so that's my guess for the first issue. Will keep reading this PR to see if there's an obvious reason for returning zeros. |
Ok had a bit of a fiddle. It seems like we create the output array using ndarray_new_dense_ndarray but don't assign the elements to it. I tried:
Which didn't give me the results I hoped for, but I have to admit I'm stumbling in the dark at the moment! Also just wondering is https://github.com/v923z/micropython-ulab/pull/651/files comparable functionality? |
Thanks for taking a look at it! I know that there is a glitch in the implementation. I'll try to fix it in the coming days. |
@Derfies Can you check out the code again? The test cases pass. |
Implemented in #688. |
Describe the solution you'd like
Implement fancy indexing based on https://numpy.org/doc/stable/reference/generated/numpy.take.html
Additional context
The implementation would resolve #607 in the most frequent use cases.
The text was updated successfully, but these errors were encountered: