-
Notifications
You must be signed in to change notification settings - Fork 916
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
fix: improve ndarray
handling of objects, nested arrays, and mixed types
#554
base: master
Are you sure you want to change the base?
Conversation
86820fd
to
2f6cc22
Compare
ndarray
handling of objects, nested arrays, and mixed types
Hi @fjosw and @j-towns, I noticed that this PR by @Gattocrucco had been lying around for a while, and I found the fix to be correct, though, I've pushed a few commits to it to make it more robust where it was lacking. Could you please review it whenever you have the time? Thank you! :) Also, thanks for your contribution, @Gattocrucco! I'm sorry that no one got towards reviewing it sooner :( I hope that you are okay with us pushing changes on top of yours. :D |
@agriyakhetarpal for me two of the five new tests already pass on master: Regarding the object arrays, I guess I don't think this is super high-priority as it's unlikely to be critically important to many users. I might have a think about whether the fix on this pr can be simplified, because atm it looks a bit complex and I don't think the benefit justifies the cost (though both are small). I do think it's nice to add the |
If we don't fix the object array issue we can add it to the tutorial section here: https://github.com/HIPS/autograd/blob/master/docs/tutorial.md#dont-use. |
Hi, I am not using autograd anymore nowadays, IIRC this bug depends on a numpy bug (numpy/numpy#16052), I would had hoped that was fixed in numpy 2, but I haven't been following. Please do away as you wish with this PR. |
Yes, forgot to mention that – I did see that the tests are passing, I've just added them to increase test coverage around this area because they were not present earlier (or I didn't find any similar test cases).
Noted! Let me revisit this over the weekend. I agree that we should check for size.
Thanks for sharing, @Gattocrucco – I'll take a read and comment on that issue upstream. That said, I am not sure if we are hitting that case in this PR. |
[The PR description has been edited by @agriyakhetarpal on 02/12/2024]: this fixes the unnecessary double wrapping for an array created from an object or a scalar value, where it is unnecessary to do so – as displayed by the reproduction in #552.
Thus, the following code (copied for completeness):
now returns
array([<object object at 0x106ec7cf0>], dtype=object)
—confirmed with the NumPy equivalent—instead of anarray([array(<object object at 0x106ec7cf0>, dtype=object)], dtype=object)
.That said, this fix does not look complete to me, so I have added additional test cases for object arrays here, as follows, which were all failing against the current
master
branch at the time of writing:(Please let me know in case there is something I missed!)