Output of `toqito/matrices/cyclic_permutation.py` is a 2D list
Closed this issue · 12 comments
Bchass I can't assign it to you because you don't show up in the suggestions, for some reason.
Let's assume, this is assigned to you.
@purva-thakre yeah, this is a silly thing that GitHub does where unless the user has commented in the thread, they won't show up. @Bchass if you comment here, you will then show up, and we can subsequently assign the issue to you.
Oh GitHub
Do we just want to change on how to call the function? Or add return np.array in the code for readability?
>>> from toqito.matrices import cyclic_permutation_matrix
>>> n = 4
>>> k = 3
>>> cyclic_permutation_matrix(n,k)
array([[0, 1, 0, 0],
[0, 0, 1, 0],
[0, 0, 0, 1],
[1, 0, 0, 0]])
>>> cyclic_permutation_matrix = cyclic_permutation_matrix(n, k)
>>> print(cyclic_permutation_matrix)
[[0 1 0 0]
[0 0 1 0]
[0 0 0 1]
[1 0 0 0]]
So the return type of the result is already of type np.ndarray. So I believe this issue is strictly a docs-related thing where we want to be explicit in the docstring. Is that correct, @purva-thakre ?
I haven't really dug into what the cause of this problem is. I created the parent issue because there is some inconsistency in the output expected by doctest vs what it got for functions that are supposed to output some type of a matrix.
If I were to use something similar to what's shown below for the expected output of cyclic_permutation_matrix, make doctest runs into a failure because it expects a 2D list not an array.
toqito/toqito/matrices/clock.py
Lines 41 to 44 in 4416bce
Sorry, I don't have a direct solution yet. Might try messing around with the output of the function by redefining it as a np.ndarray.
I'll see what else I can come up with
This is definitely not a straight forward one. Even when do I get the output to match what is expected, it still fails. Maybe whitespace? I'm still chugging away on this.
Even when do I get the output to match what is expected, it still fails. Maybe whitespace?
If you create a draft PR, we could talk about this more.
Yes, having a specific PR would be helpful as we'd be able to see where/how the failure is occurring. I recall us both working with this before and the whitespace checks are a bit annoyingly specific, so that may be what is going on here as an initial guess.
I recall us both working with this before and the whitespace checks are a bit annoyingly specific, so that may be what is going on here as an initial guess.
Not really. I locally tested the changes in one of the examples in #486. The branch for this PR is not up to date but either way, simply sandwiching the expected output using array() is not going to fix it.
Document: autoapi/matrices/cyclic_permutation/index
---------------------------------------------------
**********************************************************************
File "autoapi/matrices/cyclic_permutation/index.rst", line 46, in default
Failed example:
print(cyclic_permutation_matrix)
Expected:
array([[0 0 0 1]
[1 0 0 0]
[0 1 0 0]
[0 0 1 0]])
Got:
[[0 0 0 1]
[1 0 0 0]
[0 1 0 0]
[0 0 1 0]]
**********************************************************************
1 items had failures:
1 of 9 in default
9 tests in 1 items.
8 passed and 1 failed.
The output of the function is a 2D list whereas we want it to be a np.ndarray.
I do not think the output data type of the function shown in the screenshot is correct. Right now, doctest is flagging only a handful of these but once we start working on #299, there will be more.
