lpython icon indicating copy to clipboard operation
lpython copied to clipboard

Fix array dimension mismatch errors

Open ubaidsk opened this issue 8 months ago • 11 comments

With the recent libasr sync, we are seeing the following issue with multi-dimensional arrays in LPython.

(lp) ubaid@DESKTOP-UR96JOC:~/lpython$ cat examples/expr2.py 
from lpython import i32
from numpy import empty, int32

def main0():
    x: i32[2, 5] = empty([2, 5], dtype=int32)
    print(x)

main0()
(lp) ubaid@DESKTOP-UR96JOC:~/lpython$ PYTHONPATH=./src/runtime/lpython python examples/expr2.py 
[[  947372589       22011           0           0 -1864968336]
 [      32530 -1864968272       32530          48           0]]
(lp) ubaid@DESKTOP-UR96JOC:~/lpython$ ./src/bin/lpython examples/expr2.py 
semantic error: Type mismatch in annotation-assignment, the types must be compatible
 --> examples/expr2.py:5:5
  |
5 |     x: i32[2, 5] = empty([2, 5], dtype=int32)
  |     ^              ^^^^^^^^^^^^^^^^^^^^^^^^^^ type mismatch ('i32[2,5]' and 'i32[10]')


Note: Please report unclear, confusing or incorrect messages as bugs at
https://github.com/lfortran/lfortran/issues.

Fixing the above should hopefully get several of our array integration tests working again.

ubaidsk avatar Mar 31 '25 06:03 ubaidsk

I can take this up tmrw if its still open by then

swamishiju avatar Mar 31 '25 13:03 swamishiju

I did find a fix but i haven't wrapped my head around why it works yet I'll put up a PR tho

edit: false alarm still doesn't work

swamishiju avatar Apr 01 '25 13:04 swamishiju

@Shaikh-Ubaid I have identified the code causing the issue but i don't know to fix it, I would appreciate if you could help

https://github.com/lcompilers/lpython/blob/1e0d3001955aca8b93685467ec511d0ffd66f35e/src/lpython/semantics/python_ast_to_asr.cpp#L8652-L8653

In the above code, the type of zero->base becomes i32[10] instead of i32[2,5]

swamishiju avatar Apr 01 '25 19:04 swamishiju

I think we need a deeper look into the handling of arrays, I think print is also broken

$ cat test.py
x: i32[2, 5]
print(x)
x[0, 4] = 1
print(x)
x[1, 4] = 1
print(x)
x[0, 3] = 1
print(x)
$ lpython test.py
0    0    0    0    0    0    0    0    0    0
0    0    0    0    0    0    0    0    1    0
0    0    0    0    0    0    0    0    1    1
0    0    0    0    0    0    1    0    1    1

pre-sync

$ lpython test.py
0 0 0 0 0
0 0 0 0 0

0 0 0 0 1
0 0 0 0 0

0 0 0 0 1
0 0 0 0 1

0 0 0 1 1
0 0 0 0 1

swamishiju avatar Apr 02 '25 04:04 swamishiju

In the above code, the type of zero->base becomes i32[10] instead of i32[2,5]

So, is it the make_ArrayBroadcast_t_util() that is changing the shape of zero from i32[2,5] to i32[10]? Can you look into previous definition of make_ArrayBroadcast_t_util() (before sync) and the new definition (after sync) and see what might have changed its working?

ubaidsk avatar Apr 02 '25 04:04 ubaidsk

The type of zero->base before make_ArrayBroadcast_t_util() is i32 after it becomes i32[10] but type of asr_assign_target is i32[2, 5]

We prolly need to see how

integer, dimension(2,5) :: x

is handled in LF (excuse my syntax if its wrong im not familiar with fortran)

swamishiju avatar Apr 02 '25 08:04 swamishiju

@Shaikh-Ubaid any reason for using arraybroadcast instead of just arrayconstructor?

swamishiju avatar Apr 08 '25 07:04 swamishiju

@Shaikh-Ubaid i was working on integration tests for string i have found an issue how should I fix it

$ cat test.py
x: str
x = "ok"
print(x)
x="abcd"
print(x)
$ lpython test.py
ok
ab

I have a pretty good idea of why this is happening. Its using "constant length fortran strings" from the small examples i've seen; like below

program string
  implicit none
  character(len=4) :: first_name
  first_name = 'John'
  print *, first_name
  first_name = 'Smith'
  print *, first_name

end program string

swamishiju avatar Apr 09 '25 10:04 swamishiju

If its smtg in asr->llvm it would need separate libasr right if im not wrong

swamishiju avatar Apr 09 '25 10:04 swamishiju

any reason for using arraybroadcast instead of just arrayconstructor?

I don't remember. It is possible that some of the integration tests requires it. You can try commenting it out and/or using array constructor in its place and see if any integration tests fail. The failed integration tests should indicate where/how the arraybroadcast helps.

i was working on integration tests for string i have found an issue how should I fix it

Is the strings issue related to this current issue on arrays? If not we should discuss it on some other thread. The example you shared is

x: str
x = "ok"
print(x)
x="abcd"
print(x)

I can think of two ways of tackling this:

  1. we add a semantic error when someone tries to assign a string of length greater than what we can store in the variable
  2. we keep the current behaviour where we simply copy that many elements of the string that we can store in our variable

It seems we already have 2 working. I don't know if we want 1 here. I would say this does not look like a priority issue. There might be several other issues that could be of higher priority. You can checkout something on lfortran. That is more actively developed and should have things of higher priority than the strings issue that you hit.

For something on lpython, you can try to get back the integration tests working that got disabled during libasr sync.

ubaidsk avatar Apr 09 '25 18:04 ubaidsk

Yes thats best discussed on a different thread, I have a fix for the array integration tests #2844

The string issue i mentioned causes some integration tests to fail, another one was returning lists i'll work on those prolly tmrw

swamishiju avatar Apr 09 '25 18:04 swamishiju