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

fix: improve ndarray handling of objects, nested arrays, and mixed types #554

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Gattocrucco
Copy link

@Gattocrucco Gattocrucco commented Apr 23, 2020

[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):

from autograd import numpy as np
    x = object()

now returns array([<object object at 0x106ec7cf0>], dtype=object)—confirmed with the NumPy equivalent—instead of an array([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:

  • against lists of single and multiple objects (nested)
  • against numeric and boolean scalar arrays
  • arrays constructed with mixed data types
  • arrays constructed out of empty lists and empty lists of lists

(Please let me know in case there is something I missed!)

@Gattocrucco Gattocrucco changed the title fix and test for #552 fix #552 Apr 23, 2020
@agriyakhetarpal agriyakhetarpal changed the title fix #552 fix: improve ndarray handling of objects, nested arrays, and mixed types Dec 2, 2024
@agriyakhetarpal
Copy link
Collaborator

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 agriyakhetarpal added this to the v1.8.0 milestone Dec 2, 2024
@agriyakhetarpal agriyakhetarpal linked an issue Dec 2, 2024 that may be closed by this pull request
@j-towns
Copy link
Collaborator

j-towns commented Dec 2, 2024

@agriyakhetarpal for me two of the five new tests already pass on master: test_zero_dim_arrays and test_object_array_empty.

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 item method to ArrayBox, but it should probably include a check that self.size == 1.

@j-towns
Copy link
Collaborator

j-towns commented Dec 2, 2024

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.

@Gattocrucco
Copy link
Author

Gattocrucco commented Dec 2, 2024

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.

@agriyakhetarpal
Copy link
Collaborator

@agriyakhetarpal for me two of the five new tests already pass on master: test_zero_dim_arrays and test_object_array_empty.

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).

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 item method to ArrayBox, but it should probably include a check that self.size == 1.

Noted! Let me revisit this over the weekend. I agree that we should check for size.

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

np.array([object()]) fails
3 participants