-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Using default_dtypes
instead of hard-coding dtypes.
#666
Conversation
default-dtypes
instead of hard-coding.default_dtypes
instead of hardcoding dtypes.
default_dtypes
instead of hardcoding dtypes.default_dtypes
instead of hard-coding dtypes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @alxmrs! Added a few comments.
Thanks for the early feedback, Tom. I'll TAL later today. |
This is looking good! BTW I just noticed that we don't enforce linting in CI (I opened #668), so you might want to install pre-commit in your dev env and run |
Thanks for pointing out how to lint. As a next step for this PR, I was going to add |
That would be great! |
I just found this useful dictionary! It's really good to know that these are defined somewhere we can access, I've been looking for just the thing w/in the array api! cubed/cubed/array_api/dtypes.py Line 78 in 5f75ba2
|
17f539a
to
8ccb343
Compare
Hey @tomwhite, I have a few questions for what types to use in my latest commit. For example, see here: On one hand, it makes sense to me that these should be In my next commit, I'm going to make these default dtypes. If that's not the way we should go, I'll just undo the commit. |
(looks like the next next step is to add more test coverage 😬 .) |
That's a very good point. The choice of dtype for intermediate values is an implementation choice - and is not tied down by the spec. I'm inclined to not change this now in this PR, and look at it when looking at a concrete case for (say) JAX on GPUs. It would be good to get this merged as a relatively straightforward change - even if some cases need follow-ups. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few more comments. As I said in another comment I would leave the changes to statistical functions (pending further investigation of types for intermediate values) so it doesn't hold up the rest.
There are a couple of index dtypes in searchsorted
and arg_reduction
that would be easy to do though.
4364d78
to
11c3441
Compare
Hey @tomwhite -- I think I may be getting ahead of myself here. I was just adding default_dtypes to For now, I'll add a commit with default_dtypes with the intention of removing it should it no longer be necessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting closer! I added a few more comments.
cubed/array_api/dtypes.py
Outdated
elif x.dtype == _complex_floating_dtypes: | ||
dtype = dtypes["complex floating"] | ||
elif x.dtype == _real_floating_dtypes: | ||
dtype = dtypes["real floating"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should remove the two cases for complex and real floating, since sum
and prod
should not upcast in this case (see https://data-apis.org/array-api/latest/API_specification/generated/array_api.sum.html#array_api.sum).
Also, nansum
should be the same as sum
. It's not in the spec, but NumPy behaves the same (version 2.1.1):
>>> np.nansum(np.array([1.0, 2.0], dtype=np.float32))
np.float32(3.0)
Locally, most of the tests pass. This adds default dtypes to newly created arrays.
… creation function and fixed something broken in my code.
…ns.py." This reverts commit 6a2f60f.
Cross applied to nan_functions.py.
Co-authored-by: Tom White <tom.e.white@gmail.com>
756c511
to
ad7c1c1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great - thanks for all your work on this @alxmrs!
Fixes #658.