First matmul implementation#366
Conversation
FrancescAlted
left a comment
There was a problem hiding this comment.
Good job! See my comments.
src/blosc2/ndarray.py
Outdated
| p1, q1 = x1.chunks[-2:] | ||
| q2 = x2.chunks[-1] | ||
|
|
||
| result = blosc2.zeros((n, m), dtype=x1.dtype) |
There was a problem hiding this comment.
Instead of using x1.dtype as the outcome, it would be better to use out_dtype = np.result_type(x1, x2).
There was a problem hiding this comment.
Also, you should pass **kwargs to blosc2.zeros() for passing NDArray compression/storage details to the output.
There was a problem hiding this comment.
The exception error was this one:
> E numpy._core._exceptions._UFuncOutputCastingError: Cannot cast ufunc 'add' output from dtype('complex128') to dtype('float64') with casting rule 'same_kind'
What if that line is receives these arguments:
result = blosc2.zeros((n, m), dtype=np.result_type(x1, x2), **kwargs)
There was a problem hiding this comment.
Yes, I was suggesting exactly that :-)
There was a problem hiding this comment.
It would be nice to add tests that combines operands with different dtypes. Also, add tests that set kwargs and check that they work (by checking e.g out.cparams.codec).
tests/ndarray/test_matmul.py
Outdated
| nb = b[:] | ||
| np_res = np.matmul(na, nb) | ||
|
|
||
| np.testing.assert_allclose(blosc2_res, np_res, rtol=1e-6) |
There was a problem hiding this comment.
I suggest to add a new test (say test_matmul_disk) where you pass filenames to operands and result via the urlpath keyword argument. See for example: https://github.com/Blosc/python-blosc2/blob/main/tests/ndarray/test_ndarray.py#L74
There was a problem hiding this comment.
I’ve added the test_matmul_disk as you suggested, passing the filenames through the urlpath argument. It's ready for review in commit 5e27651.
First implementation of matmul function along with its tests and the documentation. The tests should pass, as they pass locally (Windows).
Incidentally, I proposed a change in the squeeze method to better adapt to the array API.