Support/Enforce dtype for empty() #2294
Conversation
Shaikh-Ubaid
changed the title
Shaikh-Ubaid
force-pushed
the
dtype_for_empty
branch
3 times, most recently
from
fd741b5
to
5bfa55e
Compare
integration_tests/array_03.py
Outdated
| for i in range(n): | ||
|
|
||
| m: Const[i32] = 10 | ||
| b: Allocatable[i32[:]] = empty((m,), dtype=int32) |
There was a problem hiding this comment.
Here m doesn't need to be constant. Instead this test is testing that it can be a runtime variable. So we need to preserve the test.
|
I think this is great. Thank you. I left an important comment to fix before we can merge. |
We handle Allocatable statement with empty() in visit_Call(). This removes the need of check_to_allocate_array() and extra checking needed for it in visit_AnnAssignUtil(), visit_Assign(). Thus, making code clean.
Shaikh-Ubaid
force-pushed
the
dtype_for_empty
branch
from
5bfa55e
to
def99d9
Compare
Shaikh-Ubaid
force-pushed
the
dtype_for_empty
branch
from
def99d9
to
d5f5dd0
Compare
| a: c64[n] = empty([n], dtype=complex64) | ||
| b: c64[n] = empty([n], dtype=complex64) | ||
| a: c64[n] = empty([n], dtype=complex128) | ||
| b: c64[n] = empty([n], dtype=complex128) |
There was a problem hiding this comment.
Good catch. It's confusing, but I think it's less confusing to use c32 and c64 to mean 32bit and 64bit complex numbers (each component), so c32 corresponds to f32 and c64 to f64. NumPy uses the size of the whole complex number (both components together). Now LPython catches the mistake, so that's good.
| @@ -16,21 +16,21 @@ def add_float(x: f32, y: f32) -> f32: | |||
|
|
|||
| def g(n: i32, a: T[n], b: T[n], **kwargs): | |||
| r: T[n] | |||
| r = empty(n) | |||
| r = empty(n, dtype=object) | |||
There was a problem hiding this comment.
This is fine for now to get it to compile. Good catch. This we will have to fix later, since this should really be just:
r: T[n] = empty(n, dtype=T)
But that requires cooperation of numpy and generics. We are improving the generics in LFortran, once we are done and have a reasonably good design, we'll update LPython as well.
tests/reference/asr-test_annassign_type_mismatch-7dac7be.stderr
Outdated
Show resolved
Hide resolved
tests/reference/asr-test_annassign_type_mismatch2-fc883f7.stderr
Outdated
Show resolved
Hide resolved
| --> tests/errors/test_dict2.py:4:7 | ||
| | | ||
| 4 | y[1] = -3 | ||
| | ^ type mismatch (found: 'i32', expected: 'str') | ||
| | ^ |
There was a problem hiding this comment.
I liked the short label, showing what is wrong.
|
What is the default for "empty" in NumPy, is it f64? Perhaps later we can allow the default as well. For now it's fine. I found a bug: #2294 (comment), so that should be fixed. Other than that, the rest are minor comments. Great job! Thanks for implementing this. I'll mark it as Draft until the bug is fixed. |
Previously, the test was using float64 (which is the default value for dtype). After providing the value of dtype=float32 the test seems to fail sometimes. Therefore, making the error bound less stricter.
Shaikh-Ubaid
force-pushed
the
dtype_for_empty
branch
from
8d78ed0
to
d0b31d5
Compare
Yes, I have it planned. |
|
Thank you so much for the approval! |
fixes #1790