-
Notifications
You must be signed in to change notification settings - Fork 3
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
Scripts for generating positive definiton inputs #23
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution. I have a few comments that I feel like should be addressed before merging into main
:
-
Instead of generating
np.rand
, consider methods that generate positive definitive matrices with high probability such as: https://math.stackexchange.com/questions/357980/how-to-generate-random-symmetric-positive-definite-matrices-using-matlab -
Consider using a computationally inexpensive method such as checking eigen values to verify positive definitive instead of cholesky itself to avoid heavy penalty for larger matrices
-
Please ensure the script doesn't end with an error and instead loops until a good matrix is generated.
-
Please use argparse module to take matrixsize as input to the script instead of hardcoding,
import numpy as np | ||
|
||
#enter matrix size | ||
matrixSize = 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoding this is not a good idea. Consider using argparse
module and use that to take input from the user on the matrix size to generate.
|
||
#1. Generates a random matrix A of size matrixSize x matrixSize. | ||
A = np.random.rand(matrixSize, matrixSize) | ||
print(A) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
print(A) for large matrixSize
would just flood the console
#2. Computes the product of matrix A and its transpose, i.e., A * A^T, storing the result in matrix B. | ||
B = np.dot(A, A.transpose()) | ||
print("B") | ||
print(B) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment for large matrixSize
for B
print("C") | ||
print(C) | ||
|
||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the script will either generate or not generate a positive definitive matrix when run so that should be in a loop until we do get a correct solution.
Also using Cholesky transformation itself to check if the generated matrix is positive definitive doesn't seem to be the correct solution especially for larger matrices. I would recommend using a cheaper (eigen value based or Gershgorin circle theorem to verify this). You can refer to Gilbert Strang's book/videos or online resources for this.
matrixSize = 4 | ||
|
||
#1. Generates a random matrix A of size matrixSize x matrixSize. | ||
A = np.random.rand(matrixSize, matrixSize) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend not generating matrices using rand here as the probability of generating a wrong matrix may get increasingly higher for larger matrices. Instead I would recommend looking into methods to generate positive definitive matrices with high probability such as translating this matlab code to python: https://math.stackexchange.com/questions/357980/how-to-generate-random-symmetric-positive-definite-matrices-using-matlab
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, I have another helper function in tiled_cholesky implementation which can generate the pd matrix using blas lib.
I suggest to leave this PR alone at this moment(I usually use this when i did performance test before).
And we can close this pr when get the better method.
Start Clean up:
Clean-up1:
In this way, we can definitely get positive-definite matrix after the verification: Line30