#66 [unitaryHACK] is_permutation by vanzilar · Pull Request #70 · vprusso/toqito

vanzilar · 2021-05-31T03:27:52Z

Description

Adds in the feature as requested. 😓 with not much time to go! 😭

Todos

😭 Was unable to test the test cases as is yet. However I ran each one in my own module and they seem to work as expected.
😄 Handles cases the mimic permutation matrices by checking each value to non-zero and non-1s as well as adding rows and columns for more accuracy
⚛️ Unsure if it handles cases where the matrix is not 2-dimensional properly

Questions

None 😸

Status

Ready to be reviewed

So excited. I was thinking I could get more in however still getting the handle of the whole quantum dev environment that has evolved over the years in concert with git.

Hoping I can contribute more here and there as time goes on, as I want to play with quantum theories and experiments to understand the universe a little better.

Hope you are getting many cool additions from many Unitary Hackers and Programmers!

codecov · 2021-05-31T03:33:20Z

Codecov Report

Merging #70 (a8ed351) into master (c12f21f) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@          Coverage Diff           @@
##           master     #70   +/-   ##
======================================
  Coverage    98.7%   98.7%           
======================================
  Files         121     123    +2     
  Lines        2366    2399   +33     
  Branches      566     574    +8     
======================================
+ Hits         2337    2370   +33     
  Misses         12      12           
  Partials       17      17

Impacted Files	Coverage Δ
toqito/matrix_props/is_permutation.py	`100.0% <100.0%> (ø)`
toqito/state_metrics/matsumoto_fidelity.py	`100.0% <0.0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c12f21f...a8ed351. Read the comment docs.

Will need to find a way to get this working on my local version,

vanzilar · 2021-05-31T03:58:28Z

I found some errors in my code and have corrected the one's I found. Also working on getting the test.py method to work locally so I can verify the tests do actually work.

vanzilar · 2021-05-31T05:02:08Z

I was able to get the tests running locally in a debug mode with nose. Working on resolving the errors found by the automated checks.

Unsure why it is failing on the automated 2nd check. Will find the cause soon.

I have seen some interpreters choke on different line return sequences so maybe this one does as well. Changing the line return syntax to match the other test cases.

paniash · 2021-05-31T06:18:22Z

@vanzilar I'd suggest changing the title to [unitaryHACK] (case-sensitive) so that the bot can detect this PR. :-)

paniash

Could you please format the respective files using black? pip install black --upgrade

and then run

black tests/test_matrix_props/ toqito/matrix_props/

vprusso · 2021-05-31T11:36:37Z

+    if not is_square(mat):
+        return False
+
+    for i in mat:


I think this whole approach for checking whether something is a permutation matrix is overcomplicating the task at hand. I would do something like:

if all(sum(row) == 1 for row in mat): return all(sum(col) == 1 for col in zip(*mat)) return False

I agree after reviewing this in detail.

This syntax is great I think as it reduces 7 lines down to 3 and is entirely equivalent to the 7 lines in terms of output with no issues I can find. Plus it might work better when going through the style checkers as a added bonus. I am going to try it this way, recheck and then submit and then I think this is done.

I played around with more changes to this idea make it more concise or readable however each way I tried actually made it more complex with no readability nor computational benefit.

Okay. Thanks for the heads up. Let me know when you've completed the feature.

vanzilar · 2021-05-31T13:23:53Z

I'd like to send a big thanks to @paniash and @vprusso for all the comments and ideas. 🥳 Thank you🥳 thank you🥳 thank you! 🥳

vprusso · 2021-05-31T13:28:29Z

I'd like to send a big thanks to @paniash and @vprusso for all the comments and ideas. 🥳 Thank you🥳 thank you🥳 thank you! 🥳

Happy to help. Keep going, and keep us posted if we can offer any clarifications. Cheers.

paniash · 2021-05-31T13:29:54Z

I'd like to send a big thanks to @paniash and @vprusso for all the comments and ideas. partying_face Thank youpartying_face thank youpartying_face thank you! partying_face

Happy to help. Keep going, and keep us posted if we can offer any clarifications. Cheers.

Likewise. And hopefully some day even I'll be contributing to this project too. :-)

and not duplicate function names so all tests are run

Every time is complies and passes tests on my local machine. Maybe the difference is in the spaces. Reformatted to a standard of 4 spaces for each level to see if that is the difference between my local tests and the remote tests.

This file appears needed and was missing. Was not required for local tests.

vanzilar · 2021-06-02T16:02:53Z

I have an update. The issue appears to be a difference between what is up in github and what is down on my local machine. I am going to change the title to [WIP].

vprusso · 2021-06-02T16:24:34Z

@vanzilar I'm not sure how that's going to solve your problems, but you're welcome to do that in addition to fixing the issues.

Ran local tests and confirmed the .is_permutation is needed in this case

vprusso · 2021-06-08T20:19:15Z

I may be able to use some help. Unsure with what yet.

Cool, let me know if you have something specific, and I'll try to provide guidance.

vanzilar · 2021-06-09T05:24:35Z

Thanks all. I have learned quite a bit. I know what the code-cov plugin does now.... it is awesome the way to zeroes in to make sure you test every check at least once.

I need to spend a drop more time now on seeing if I can understand the proposed change that was sent by using zip and * or perhaps to find a more concise way to check for permutation matrices, using functions I have not tried in the pymath package or elsewhere in toqito itself. Will send any questions up as soon as I have one.

vprusso · 2021-06-09T16:10:05Z

Will send any questions up as soon as I have one.

Cool, keep me posted!

Change the 7 lines of code to 3.

vanzilar · 2021-06-10T21:12:03Z

OK all done from my side now. :) Ready for any comments or changes however.....

Side note:

From a personal standpoint, I am still thinking about a more concise way to do:
for i in mat:
for j in i:
if (j not in (0, 1)):
return False

However I have not found a better way with python or pymath syntax yet. I am open to better suggestions. My Computer Science friends often prefer this type of broken up form however so they can see the check on every element.

There should be a simpler way to iterate with one interable through every element in a matrix in python and run a check on each.

Wait a second... there is an easier way.... ok tested... one more commit coming in.

Removed the 2 comments and the parenthesis as per pylint's preferences. all good.

vanzilar · 2021-06-10T21:50:27Z

OK now I am done. (There is always more to change) however.

Let me explain my logic here. Based on the wikipedia def of a permutation matrix only 0s and 1s are allowed. To be explicit I put in an is_square even though it might be superfluous as all valid permutation matrices will be square given the other checks.
I can't think of an example of a non-square matrix that can pass the next other two checks... however I might be missing one, so I left in that check.

I found just adding up rows and columns there are many matrices that can add up to 1 in every row and column and not consist of 0s and 1s, so I had to check each element to make sure it is in the set of 0,1., (see tests for a few examples)

So I am going to leave my submission as is and ask for any more comments or requested changes and advice.

vprusso · 2021-06-10T22:14:57Z

+    >>> is_permutation(A)
+    True
+
+    Alternatively, the following example matrix :math:`B` defined as


This sentence isn't complete, you don't mention whether or not the following matrix B is a permutation matrix or not.

vprusso · 2021-06-10T22:18:42Z

Thanks @vanzilar. Added some comments.

Changes the matrix notation from indicating index numbers to containing the actual element values as requested. Removed is_square check as requested. (If it is needed it can be restored later)

vanzilar · 2021-06-11T00:48:03Z

Running into a end-case issue. When I remove is_square check... and try to check the single value matrix [[1]] I am getting errors. For this trivial matrix should it be considered a permutation matrix of the identity variety or a non-permutation matrix?

Unsure how to proceed or how it should be defined in this context.

Here is the test that is failing

def test_is_one_dimensional_permutation():
"""Test if matrix is a 1-D permutation matrix."""
mat = np.array([1])
np.testing.assert_equal(is_permutation(mat), False)

Or should it be true?

also debating whether to add a 1-D permutation matrix check and if it is valid or not.

vprusso · 2021-06-11T00:56:45Z

Running into a end-case issue. When I remove is_square check... and try to check the single value matrix [[1]] I am getting errors. For this trivial matrix should it be considered a permutation matrix of the identity variety or a non-permutation matrix?

Unsure how to proceed or how it should be defined in this context.

Here is the test that is failing

def test_is_one_dimensional_permutation():
"""Test if matrix is a 1-D permutation matrix."""
mat = np.array([1])
np.testing.assert_equal(is_permutation(mat), False)

Or should it be true?

As mentioned in the previous comment, you don't need the is_square check, you can just remove it here.

Was causing compile error.

vanzilar · 2021-06-11T01:19:04Z

OK all done... let me know of the 2 remaining topics:

if you find any errors or function names to rename.
For a 1-Dimensional 1-Element matrix with a value of 1 (essentially a scalar of 1) if that should be considered as a permutation matrix or not.

Removing is the best way to demonstrate why this additional code for checking each element is necessary, Without checking if each element is in the set, 0,1 the checks for various non-permutation matrices that mimic permutation matrices but are not permutation matrices per the wikipedia definition, will fail in the testing suite.

Added spaces Also modified import statement to match the new line of code in the init py file in the toqito directory

vanzilar · 2021-06-11T12:57:00Z

OK all the changes are in now.

The build fails as designed.

Next step is:
Are the tests I designed correct (in which case we need to add back in the (0,1) element check)?
OR
Is the definition of the permutation matrix wrong, in which case, I need to change the comments and change the results of the test cases to match? (leaving the element check off)

Let me know which way is correct.

vprusso · 2021-06-11T13:04:00Z

OK all the changes are in now.

The build fails as designed.

Next step is:
Are the tests I designed correct (in which case we need to add back in the (0,1) element check)?
OR
Is the definition of the permutation matrix wrong, in which case, I need to change the comments and change the results of the test cases to match? (leaving the element check off)

Let me know which way is correct.

Ah gotcha. No, I think you were correct and I mistakenly told you to remove the for loop check in your code. I think that is necessary to keep in there, so apologies for the confusion. So, place that back in and make sure the test suites pass.

vanzilar · 2021-06-11T15:44:00Z

All in now. Let me know if this is good or if there are more changes to make.

Thanks so much for all the checks.

While reviewing the recommended fixes found this was spelled wrong

vprusso · 2021-06-11T18:06:36Z

Cool, accepted.

vanzilar added 4 commits May 30, 2021 23:06

Adding in a basic is-permutation matrix checker

c167e0b

Adding test cases for the is_permutation checked

b23b2dd

Update to add in is_permutation

02f62d3

Merge branch 'vprusso:master' into is-permutation-matrix

4e265d7

vanzilar added 2 commits May 30, 2021 23:52

Spelling error fixed

87933e2

Correcting a function call error

d4c0f6c

Will need to find a way to get this working on my local version,

Removed unnecessary import statement

27da1eb

vanzilar added 2 commits May 31, 2021 01:30

More grammar and spelling corrections in comments.

8b9c5b2

Unsure why it is failing on the automated 2nd check. Will find the cause soon.

try changing trailing line returns..

e6f56bd

I have seen some interpreters choke on different line return sequences so maybe this one does as well. Changing the line return syntax to match the other test cases.

paniash suggested changes May 31, 2021

View reviewed changes

Comment thread toqito/matrix_props/is_permutation.py

Comment thread toqito/matrix_props/is_permutation.py Outdated

vprusso reviewed May 31, 2021

View reviewed changes

vanzilar changed the title ~~#66 [unitaryHack] is_permutation~~ #66 [unitaryHACK] is_permutation May 31, 2021

vanzilar added 5 commits June 1, 2021 19:55

Making recommended corrections and a few more

612afcc

Fix test function names and comments to be clearer

387df95

and not duplicate function names so all tests are run

Try changing python spaces

584a004

Every time is complies and passes tests on my local machine. Maybe the difference is in the spaces. Reformatted to a standard of 4 spaces for each level to see if that is the difference between my local tests and the remote tests.

Merge branch 'vprusso:master' into is-permutation-matrix

1d2456d

Create toqito.matrix_props.is_permutation.rst

668ac31

This file appears needed and was missing. Was not required for local tests.

vanzilar changed the title ~~#66 [unitaryHACK] is_permutation~~ #66 [unitaryHACK] is_permutation [WIP] Jun 2, 2021

vanzilar added 2 commits June 2, 2021 12:42

Adding .is_permutation suffix to import statement

bb14bb5

Ran local tests and confirmed the .is_permutation is needed in this case

missing \\ added to comment section

5ee7944

Added in 2 more tests to reach complete coverage of tests

232af52

Reduce line count per recommendation

a86af9d

Change the 7 lines of code to 3.

vanzilar changed the title ~~#66 [unitaryHACK] is_permutation [WIP]~~ #66 [unitaryHACK] is_permutation Jun 10, 2021

vanzilar added 2 commits June 10, 2021 17:16

Add 2 comments and simplify the (0,1) check

20fac42

pylint prefers no comments and less parenthesis.

f280e32

Removed the 2 comments and the parenthesis as per pylint's preferences. all good.

vprusso reviewed Jun 10, 2021

View reviewed changes

Changed matrix notation, removed square check

45e7e1b

Changes the matrix notation from indicating index numbers to containing the actual element values as requested. Removed is_square check as requested. (If it is needed it can be restored later)

Rename functions

9b1bba9

also debating whether to add a 1-D permutation matrix check and if it is valid or not.

Removed one space in a comment

dd2484c

Was causing compile error.

vprusso reviewed Jun 11, 2021

View reviewed changes

vanzilar added 4 commits June 11, 2021 08:16

Adding is_permutation

277b00c

Migrating code to conform closer to PEP-8 notation

910b83d

Added spaces Also modified import statement to match the new line of code in the init py file in the toqito directory

PEP-8 formatting changes

f5d4360

Undo (0,1) check removal

9656908

Correcting inadvertent spelling error

a8ed351

While reviewing the recommended fixes found this was spelled wrong

vprusso merged commit 57e09c0 into vprusso:master Jun 11, 2021

Conversation

vanzilar commented May 31, 2021

Description

Todos

Questions

Status

Uh oh!

codecov bot commented May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vanzilar commented May 31, 2021

Uh oh!

vanzilar commented May 31, 2021

Uh oh!

paniash commented May 31, 2021

Uh oh!

paniash left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vprusso May 31, 2021

Choose a reason for hiding this comment

Uh oh!

vanzilar Jun 10, 2021

Choose a reason for hiding this comment

Uh oh!

vprusso Jun 10, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vanzilar commented May 31, 2021

Uh oh!

vprusso commented May 31, 2021

Uh oh!

paniash commented May 31, 2021

Uh oh!

vanzilar commented Jun 2, 2021

Uh oh!

vprusso commented Jun 2, 2021

Uh oh!

vprusso commented Jun 8, 2021

Uh oh!

vanzilar commented Jun 9, 2021

Uh oh!

vprusso commented Jun 9, 2021

Uh oh!

vanzilar commented Jun 10, 2021

Uh oh!

vanzilar commented Jun 10, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vprusso Jun 10, 2021

Choose a reason for hiding this comment

Uh oh!

vanzilar Jun 11, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vprusso commented Jun 10, 2021

Uh oh!

vanzilar commented Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vprusso commented Jun 11, 2021

Uh oh!

vanzilar commented Jun 11, 2021

Uh oh!

Uh oh!

codecov bot commented May 31, 2021 •

edited

Loading

paniash left a comment •

edited

Loading

vanzilar commented Jun 11, 2021 •

edited

Loading