phanrahan / magma

magma circuits
Other
239 stars 22 forks source link

[Discussion] Magma N-D Arrays have unintuitive behavior #1310

Closed rkshthrmsh closed 6 months ago

rkshthrmsh commented 11 months ago

(This is probably better suited to be a discussion. Putting it here for now.)

As per the documentation, it appears that the shape tuple used to initialize Magma N-D arrays has reverse axis ordering compared to Numpy N-D arrays.

arr_np = np.ones((3, 2))
arr_np.shape
# (3, 2)

arr_m = m.Array[(3, 2), m.Bits[4]]([[1] * 3] * 2) # initialize to ones
# has shape (2, 3) <-- unintuitive

However, the indexing order follows that of Numpy.

arr_np[0] # access row 0
# has shape (2,)

arr_m[0] # access row 0
# has shape (3,)

Finally, the indexing syntax mentioned in the documentation doesn't seem to work with Magma N-D arrays:

>>> arr_np[0, 0]
1.0
>>> arr_m[0, 0]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/rkshthrmsh/ws/magma/magma/array.py", line 811, in __getitem__
    return self._ndarray_getitem(key)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/rkshthrmsh/ws/magma/magma/array.py", line 767, in _ndarray_getitem
    return self[key[-1]][key[:-1]]
           ~~~~~~~~~~~~~^^^^^^^^^^
TypeError: list indices must be integers or slices, not tuple
leonardt commented 11 months ago

I think the indexing issue may be a bug related to constants, will investigate that.

For the ordering, I'll have to revisit the design and see if I can figure out why it was reversed, it may be related to endianness and indexing often being reversed in the HDLs (e.g. verilog slicing 78:0]) when compared to traditional programming language.

leonardt commented 6 months ago

Actually going to keep this open to track that issue with indexing the constant and make sure it gets fixed before closing. Try out the new declaration/indexing order and see if it matches what you'd expect now