ENH: add a float32-only device#206
Conversation
5cd3619 to
6ba0f5b
Compare
|
I had a quick look at the diff, I think this is going in a direction I like. One thought to which I have no good answer: would it make sense to repurpose Interested what other think about this. |
|
I am fine with repurposing |
|
I think it would make sense for the float32 only device to raise an exception on Note that this is not the case for PyTorch on the MPS devices for some reason: >>> import torch
>>> a = torch.ones(10, device="mps")
>>> b = 2 * torch.ones(10) # CPU
>>> a + b
Traceback (most recent call last):
Cell In[9], line 1
a + b
RuntimeError: Expected all tensors to be on the same device, but found at least two devices, mps:0 and cpu!
>>> b[:5] = a[:5] # implicit MPS to CPU transfer does not raise!
>>> b
tensor([1., 1., 1., 1., 1., 2., 2., 2., 2., 2.])I would have expected the last statement to also raise EDIT: actually, this is not related to dtypes. Maybe I should open a dedicated issue for this. EDIT2: done: #207 |
|
Okay, this is ready from my side. Re yet another device vs repurpose device2, the latter is more churn; the former works today and a new device is free, so am opting to adding a new device unless someone has a strong opinion to the contrary. |
As a part of a rebase/conflict resolution, this commit simply moves the dlpack/device updates to _devices.py. Co-authored-by: Tim Head <betatim@gmail.com>
|
This indeed seems useful, thanks for pushing this forward!
Also MLX on GPU: https://ml-explore.github.io/mlx/build/html/python/data_types.html |
betatim
left a comment
There was a problem hiding this comment.
I like having this device! Here some review comments.
We need some kind of nice name for this device
| _complex_floating_dtypes, _numeric_dtypes | ||
| ) | ||
|
|
||
| _ALL_DEVICE_NAMES = ("CPU_DEVICE", "device1", "device2", "F32_device") |
There was a problem hiding this comment.
How about spelling out the name of the device a bit more?
My first thought was "float32_only_device" but I think that is confusing because you might think it means the only type available is float32. So maybe no_float64_device is more obvious. I do wonder if we could call it just no_float64, the device postfix feels a bit superfluous, but it does make the string meaningful by itself.
| _ALL_DEVICE_NAMES = ("CPU_DEVICE", "device1", "device2", "F32_device") | |
| _ALL_DEVICE_NAMES = ("CPU_DEVICE", "device1", "device2", "no_float64_device") |
There was a problem hiding this comment.
Thanks! Since the vast majority of usage is xp.Device("no_floa64") with the word Device already used, I went with no _device" postfix in the updated PR. If xp.Device("no_float64_device")` reads better to you @betatim , I'll change to that.
[a totally unneccessary explanation: English is a second language for me, and my first language has a rather strong preference for not repeating words even at a cost of breaking the word order in a sentence, thus the choice above; all that said, I'm not going to argue the point if you prefer Device("no_float64_device")]
| if dtype is None and device is not None: | ||
| res_dtype = DType(res.dtype) | ||
| if not device_supports_dtype(device, res_dtype): | ||
| # find out the default dtype for the device |
There was a problem hiding this comment.
I think this does the right thing, but I was surprised by it. I was expecting the code to find the default dtype for the device, given that no dtype was passed to asarray, and then create the Numpy array with that information. Instead we first create the Numpy array, then check if the dtype it selected is supported by the device. This feels "backwards". I think most of that is because in my head I'm thinking "whether a device is supported or not, and if it is the default device or not are two different questions". As in, on some fictitious device the default dtype chosen by Numpy is supported, but not the default. Then this code would no longer do the right thing.
I think the reason the code is the way it is, is that without the help of np.array we'd have to work out which group of dtypes the input belongs to. For example in the case of array_api_strict.asarray([1,2,3.141], device=foobar) we'd have to determine that we need the floating point dtypes and for array_api_strict.asarray([1,2,3], device=foobar) we need the integer dtypes.
Maybe we can get the best of both worlds by keeping the current code and adding a check that the selected res_dtype is a allowed default dtype for device.
I think the below comment represents the current thinking and explains a bit what is happening here. I'd probably add this no matter what (unless we completely rewrite all this):
| # find out the default dtype for the device | |
| # The dtype selected by Numpy might not be the default dtype | |
| # on this device. If the dtype is not supported by the device we | |
| # try to find one that is, aka the default dtype of this device. |
There was a problem hiding this comment.
Agreed that the code is a bit awkward. And indeed I'd rather lean on numpy and correct afterwards and avoid scanning the inputs to find out the dtype from scratch which can be bit of a rabbit hole, too.
As in, on some fictitious device the default dtype chosen by Numpy is supported, but not the default. Then this code would no longer do the right thing.
So that would be a pytorch "cpu" device with the default pytorch settings, which supports float64 but defaults to float32. That said, I don't know if we need to go all the way here?
For now, I kept the adjust-the-dtype dance and added your explanatory comment, hoping that that'd be a reasonable compromise?
| return full_like(a, fill_value=42.0, *args, **kwds) | ||
|
|
||
|
|
||
| class TestDefaultDType: |
There was a problem hiding this comment.
Did you pick the class based testing approach on purpose? I often have AI do this, even if all of the rest of the project uses test functions. Unless there is a good reason for this I'd vote for keeping it consistent and using test functions. Not classes
There was a problem hiding this comment.
It's just my default mode (that it matches what AI does is by accident, I swear I'm made of meat and bone :-)) to group related tests into a class, since I find it easier to navigate a test module with a dozen classes with a dozen test functions each, than a flat list of 144 test functions. Here for example, TestDefaultDType::test_eye clearly tests the eye function for its default dtype behavior, as opposed to a module-level test_eye which tests the eye function yes, but I need to read the test to find out which behaviors of this function are being tested.
So whether it's on purpose is yes and no: no I did not give it much thought, yes I've a mild preference towards this style; the preference is mild indeed, so if this irks the reviewer, I'm happy to remove the test class. Yay/Nay?
Co-authored-by: Tim Head <betatim@gmail.com>
|
Thanks for the review @betatim ! I pushed an update which addressed most of your comments. Also took liberty to resolve those GH comments where an update seemed to just go with your suggestions, so that they don't distract from the few comments which are less than clear-cut and I'd be looking for more input. |
Add a new device which only supports single precision floats and does not support double precision floats.
The new device mimics torch "mps" device in that it does not have f64 but supports int64---unlike JAX which either has both or none.
dtypes,default_dtypes) device-aware;ones,emptyetc) use the device-specific default dtype when givendtype=None, device=f32_deviceTODO:
fft.{fftfreq, rfftfreq}device=arguments in internal constructionsTBD:
device2or add a newF32_device(if so, bikeshed the name)dtype=float64, device=f32_only_deviceshould raise? torch "mps" tensors raise a TypeError, follow it or mandate a ValueError?Intends to close gh-64,
Gives a way to close gh-38 --- if we have a f32-only device, we probably do not need a global flag
Addresses a large part of gh-70
Cross-ref the spec RFC to allow for missing dtypes , data-apis/array-api#998 --- note that this
array-api-strictPR can only land after the spec is updated;Also cross-ref the test suite tracker data-apis/array-api-tests#431: the test suite is actually fairly far along.
EDIT: data-apis/array-api#1005 is I believe the spec update to make this spec-compliant.