-
Notifications
You must be signed in to change notification settings - Fork 80
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
Add is_etf #999
base: master
Are you sure you want to change the base?
Add is_etf #999
Conversation
toqito/matrix_props/is_etf.py
Outdated
@@ -0,0 +1,62 @@ | |||
'''Checks if matrix forms an equilangular tight frame (ETF).''' |
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.
You should use """
not '''
'''Checks if matrix forms an equilangular tight frame (ETF).''' | ||
import numpy as np | ||
from toqito.matrix_ops import vectors_to_gram_matrix | ||
|
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.
Two spaces between imports and function definition.
toqito/matrix_props/is_etf.py
Outdated
r"""Check if matrix constitutes an equilangular tight frame. | ||
|
||
Args: | ||
mat(array): A matrix of any size |
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.
You are missing arguments here in the description. Also, you don't need to put the type in the docstring if you supply it in the argument. Additonally, the current type is incorrect.
You also need a period at the end of sentences.
toqito/matrix_props/is_etf.py
Outdated
mat(array): A matrix of any size | ||
|
||
Definition taken from: | ||
:cite:`http://users.cms.caltech.edu/~jtropp/conf/Tro05-Complex-Equiangular-SPIE-preprint.pdf`. |
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.
Please put citations in the .bib
file as is done in other functions.
toqito/matrix_props/is_etf.py
Outdated
|
||
A matrix A of any size constitutes an equilangular tight frame if it satisfies three conditions: | ||
1) If each of the columns of the matrix has unit norm. | ||
2) If all the diagnol elements of the gram matrix is one and its off-diagnol elements are constant. |
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.
diagnol -> diagonal
toqito/matrix_props/is_etf.py
Outdated
return False | ||
|
||
|
||
#Checks if the matrix is equiangular |
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.
Space after #
@@ -0,0 +1,39 @@ | |||
"""Test is_etf.""" |
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.
Do these tests cover every possible branch?
toqito/matrix_props/is_etf.py
Outdated
import numpy as np | ||
from toqito.matrix_ops import vectors_to_gram_matrix | ||
|
||
def is_etf(mat: np.ndarray, rtol: float = 1e-05, atol: float = 1e-08) -> bool: |
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.
Why are these rtol
and atol
supplied if they aren't used?
"""Test is_etf.""" | ||
|
||
import numpy as np | ||
from toqito.matrix_ops import vectors_to_gram_matrix |
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.
Why is this being imported?
"""Test that a matrix whose columns have unit norm but are not equiangular returns False.""" | ||
# Each column has unit norm but the off-diagonal of the Gram matrix isn't constant. | ||
mat_unit_but_not_equiangular = np.array([ | ||
[1, 0, 1/np.sqrt(2)], |
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.
Don't add spacing as this is hard to maintain and is unnecessary for this matrix.
Description
Changes
Checklist
Before marking your PR ready for review, make sure you checked the following locally. If this is your first PR, you might be notified of some workflow failures after a maintainer has approved the workflow jobs to be run on your PR.
Additional information is available in the documentation.
ruff
for errors related to code style and formatting.pytest
.Sphinx
build can be checked locally for any failures related to your PRlinkcheck
to check for broken links in the documentationdoctest
to verify the examples in the function docstrings work as expected.