Skip to content

ENH: new function isclose #113

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Jan 21, 2025
Merged

ENH: new function isclose #113

merged 7 commits into from
Jan 21, 2025

Conversation

crusaderky
Copy link
Contributor

No description provided.

@lucascolley lucascolley added enhancement New feature or request new function labels Jan 20, 2025
@lucascolley lucascolley self-requested a review January 20, 2025 16:51
lucascolley
lucascolley reviewed Jan 20, 2025
View reviewed changes
Copy link
Member

@lucascolley lucascolley left a comment

Choose a reason for hiding this comment

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

Please could you read through the discussion at data-apis/array-api#170 and let me know how this PR stands in relation to that?

@crusaderky
Copy link
Contributor Author

crusaderky commented Jan 21, 2025 •
edited
Loading

Please could you read through the discussion at data-apis/array-api#170 and let me know how this PR stands in relation to that?

This is 1:1 what numpy, cupy, tensorflow, pytorch, jax and dask already implement.

  • it uses the controversial, but backwards compatible, numpy algorithm instead of the math.isclose one.
  • it uses the libraries' default atol and rtol (which are problematic for small numbers) instead of the better ones from math.isclose, again for the sake of backwards compatibility.
  • it doesn't tweak tolerances for float32, like xp_assert_close does, for the same reason.
  • it applies the same default tolerances to integers. I found myself in several occasions to find this useful.
  • it treats bools as integers, like numpy does. This is IMHO unnecessary but I had to special-case it either way (either in _func to respect numpy's behaviour or in _delegation to deviate from it even when delegation is normally possible). I think it's enough of an edge case that either decision is inconsequential.
  • there is discussion in the RFC about the usefulness of equal_nan=False, with consensus veering on it being frequently very useful. The RFC fails to highlight that you need scipy's lazywhere in order to suppress the warnings. Again, this PR implements what numpy does; however for array-api-strict (and possibly other non-special-cased libraries that wrap around numpy) you temporarily still have spurious warnings. I will fix this in the PR that adds lazywhere.
  • inf behaviour is the same as in all libraries as well as in math.isclose and in line with the recommendations of the RFC (inf is only close to itself, as is -inf).

The RFC is right to point out though that allclose is trivial and overkill to implement. I've removed it from this PR.

@crusaderky crusaderky changed the title ENH: new functions isclose and allclose ENH: new function isclose Jan 21, 2025
crusaderky
crusaderky commented Jan 21, 2025
View reviewed changes
src/array_api_extra/_lib/_funcs.py

if xp.isdtype(a.dtype, "bool") or xp.isdtype(b.dtype, "bool"):
if atol >= 1 or rtol >= 1:
return xp.ones_like(a == b)
Copy link
Contributor Author

@crusaderky crusaderky Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

On eager backends, this is less performant than

return xp.ones(xp.broadcast_arrays(a, b)[0], dtype=bool, device=a.device)

but it supports backends with NaN shapes like Dask.
Both jax.jit and dask with non-NaN shape should elide the comparison away.

crusaderky
crusaderky commented Jan 21, 2025
View reviewed changes
src/array_api_extra/_lib/_testing.py
desired_shape = desired.compute().shape

msg = f"shapes do not match: {actual_shape} != f{desired_shape}"
assert actual_shape == desired_shape, msg
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will have to be replicated on the scipy PR for dask support

@rgommers
Copy link
Member

This is 1:1 what numpy, cupy, tensorflow, pytorch, jax and dask already implement.

That seems like the right choice to me for array-api-extra.

@lucascolley lucascolley self-requested a review January 21, 2025 10:21
lucascolley
lucascolley reviewed Jan 21, 2025
View reviewed changes
Copy link
Member

@lucascolley lucascolley left a comment

Choose a reason for hiding this comment

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

this looks fantastic as usual, thanks! I'll push some tweaks now.

@lucascolley
Copy link
Member

happy to merge once unresolved comments are addressed

@lucascolley lucascolley added this to the 0.6.1 milestone Jan 21, 2025
@crusaderky
Copy link
Contributor Author

happy to merge once unresolved comments are addressed

all done

lucascolley
lucascolley approved these changes Jan 21, 2025
View reviewed changes
Copy link
Member

@lucascolley lucascolley left a comment

Choose a reason for hiding this comment

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

thanks!

@lucascolley lucascolley merged commit 6ee70c0 into data-apis:main Jan 21, 2025
8 checks passed
@crusaderky crusaderky deleted the isclose branch January 21, 2025 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucascolley lucascolley lucascolley approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request new function
Projects
None yet
Milestone
0.6.1
Development

Successfully merging this pull request may close these issues.
3 participants
@crusaderky @rgommers @lucascolley