Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

matrix vector multiplication broken in ulab #4250

Closed
kevin-tracy opened this issue Feb 23, 2021 · 14 comments · Fixed by #4410
Closed

matrix vector multiplication broken in ulab #4250

kevin-tracy opened this issue Feb 23, 2021 · 14 comments · Fixed by #4410
Labels
bug ulab Related to the ulab module

Comments

@kevin-tracy
Copy link

Matrix vector multiplication isn't working in ulab on the following version:
Adafruit CircuitPython 6.1.0 on 2021-01-21; Adafruit Grand Central M4 Express with samd51p20

Here is an example of it's current functionality:

import ulab as np
A = np.eye(3)
B = np.array([1,2,3.0])

# the following two lines don't work 
C = np.linalg.dot(A,B)                    # ValueError: matrix dimensions do not match
C = np.linalg.dot(A,B.transpose())        # ValueError: matrix dimensions do not match

# this does work, but the resulting array C is a 2D column vector 
C = np.linalg.dot(A,B.reshape((3,1)))
C = C.flatten()                           # flatten to 1D vector

Ideally, this would work similarly to NumPy where a 2D array multiplied by a 1D array results in a 1D array. Does this functionality exist under a different function? Thank you.

@jepler
Copy link
Member

jepler commented Feb 23, 2021

@v923z I reproduced the same in the legacy branch of ulab as well as in master branch. Can you please look into it? If possible we'd like it fixed in legacy so we can take the fix in 6.2.

@v923z
Copy link

v923z commented Feb 23, 2021

@jepler Sure! What is the deadline? I should be able to fix it tomorrow.

@v923z
Copy link

v923z commented Feb 23, 2021

@jepler @kevin-tracy v923z/micropython-ulab#339 should hopefully fix the issue.

@kevin-tracy
Copy link
Author

@v923z thank you very much! Out of curiosity, how come that change was made to the micropython-ulab repo instead of circuitpython-ulab?

@v923z
Copy link

v923z commented Feb 23, 2021

@kevin-tracy Because @jepler simply links to micropython-ulab, so it makes sense to implement all fixes there. I think, having the same thing at two different places is generally not a sound idea.

@jepler
Copy link
Member

jepler commented Feb 24, 2021

Initially we thought we might have to make local changes to allow ulab to work with circuitpython but v923z has been very responsive to all our needs, keeping the changes in his repo. It is working out very well!

@v923z
Copy link

v923z commented Feb 24, 2021

@kevin-tracy I forgot to ask, do you think you could write a test script for linalg.dot? We should have been able to catch this error earlier, and the fact that we didn't means that the test was not quite up to the task... In case you are interested, here is what you would need: https://github.com/v923z/micropython-ulab/#testing

@kevin-tracy
Copy link
Author

@v923z I would be happy to! I'll add them sometime this week.

@v923z
Copy link

v923z commented Feb 24, 2021

@kevin-tracy Many thanks! @jepler Jeff, I hope that my appeal for testing doesn't qualify as hijacking the thread.

@kevin-tracy
Copy link
Author

@v923z this might not be the right place to ask this question, but in writing tests I'm having an issue generating the .exp file for a new test. In the readme of the repo it says:

In case you have a new snippet, i.e., you have no expected results file, or if the results differ from those in the expected file, a new expected file will be generated in the root directory.

But when I do this for a new new_tests_2.py, I get the following error:

1 tests failed: new_tests_2
-e 
FAILURE *
diff: *.exp: No such file or directory
diff: *.out: No such file or directory

Am I misunderstanding something or should I make an issue for this? Thanks.

@kevin-tracy
Copy link
Author

Two quick updates to the above comment:

  1. It looks like when a test script results in a different output than expected, it creates the appropriate .exp file in the root directory. However, when there is no expected output .exp file, it does not produce the appropriate .exp file in the root directory.
  2. It also looks as if the matrix-vector multiplication is still not working. I tried both with the testing tool on my computer as well as the most recent version on my board:
Adafruit CircuitPython 6.2.0-beta.2-160-g5158aec8c on 2021-02-26; Adafruit Grand Central M4 Express with samd51p20
>>> import ulab as np
>>> A = np.eye(3)
>>> b = np.array([1,2,3])
>>> np.linalg.dot(A,b)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: matrix dimensions do not match

On my board I am running 20210226-5158aec.uf2 from here.

@v923z
Copy link

v923z commented Feb 27, 2021

@kevin-tracy Sorry, I didn't spell this out, but anything written on micropython-ulab is for micropython.

But more importantly, the fix is in a still-open PR, v923z/micropython-ulab#339, so Jeff hasn't yet pulled in the changes, because the PR is still not resolved. Hence, loading an adafruit-compiled piece of firmware is not going to make the cut here.

What you can do, however, is simply clone https://github.com/v923z/micropython-ulab/, switch to the legacy-dot branch, and then run the build script. That will clone micropython, and run the tests. If you are happy with the test results, then you can PR against that branch.

@jepler jepler mentioned this issue Mar 15, 2021
@jepler jepler added bug ulab Related to the ulab module labels Mar 15, 2021
@jepler jepler added this to the 6.x.x - Bug Fixes milestone Mar 15, 2021
@amitkrbhuyan
Copy link

Is the matrix multiplication issue fixed for circuitpython?

@jepler
Copy link
Member

jepler commented May 25, 2021

@amitkrbhuyan we believe it is fixed in the current stable release, 6.2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug ulab Related to the ulab module
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants