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: add ak.array_equal to NEP18 overrides, documentation, and add one more test #3225

Merged
merged 3 commits into from
Aug 28, 2024

Conversation

tcawlfield
Copy link
Collaborator

  • Now overrides numpy.array_equal
  • Adding link to toctree
  • Unit test checks numpy override

* Now overrides numpy.array_equal
* Adding link to toctree
* Unit test checks numpy override
@tcawlfield tcawlfield self-assigned this Aug 23, 2024
@jpivarski jpivarski changed the title fix: Missed changes with ak.array_equal doc: add ak.array_equal to documentation and add one more test Aug 23, 2024
Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good; I agree that the heading :caption: should be "approximation and comparison".

The first set of test failures are due to the fact that Awkward only supports native-endian numbers, and you have some tests that make big-endian unsigned int32. I don't know what the second issue is.

Once those are fixed, this is mergeable!

@jpivarski jpivarski changed the title doc: add ak.array_equal to documentation and add one more test fix: add ak.array_equal to NEP18 overrides, documentation, and add one more test Aug 23, 2024
@tcawlfield
Copy link
Collaborator Author

tcawlfield commented Aug 23, 2024

The first set of test failures are due to the fact that Awkward only supports native-endian numbers, and you have some tests that make big-endian unsigned int32. I don't know what the second issue is.

Oh, a regression failure! I'm seeing the first of these errors on my own system once I run all tests. Hmm. The failure is here.

almost_equal is getting the layout of both inputs sooner than it really can in the case that one array is a pure numpy array. Maybe I need to add some earlier tests of that condition -- if one (or both I suppose) array is raw numpy (or cupy or jax etc), then do a simpler set of tests. I could also try/except and ... no, that won't work. Or maybe only run to_layout(...) on Array instances, and only to_packed() on particular layout classes?

I'll work on this.

Using ensure_same_backend now, which raises an exception instead of
returning False. This is a new behavior for ak.almost_equal.
This also removes a shortcut in the code, and we get a NotImplementedError
on a typetracer backend. Test_2678 updated to match this expectation.
Copy link
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense (the update in ec93fd2): "same backend" is a broader criterion than someone is likely to be asking about when they ask whether two arrays are equal. Having it raise an error, like all of the other functions, is good for consistency.

@tcawlfield tcawlfield merged commit c30c14e into main Aug 28, 2024
41 checks passed
@tcawlfield tcawlfield deleted the array-equal-override-and-docs branch August 28, 2024 17:33
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.

2 participants