-
Notifications
You must be signed in to change notification settings - Fork 2
[WIP] Define random_unitary
constructor
#39
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #39 +/- ##
==========================================
- Coverage 95.02% 87.55% -7.47%
==========================================
Files 15 17 +2
Lines 422 458 +36
==========================================
Hits 401 401
- Misses 21 57 +36
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ext/TensorAlgebraGradedUnitRangesExt/TensorAlgebraGradedUnitRangesExt.jl
Outdated
Show resolved
Hide resolved
ext/TensorAlgebraGradedUnitRangesExt/TensorAlgebraGradedUnitRangesExt.jl
Outdated
Show resolved
Hide resolved
ext/TensorAlgebraGradedUnitRangesExt/TensorAlgebraGradedUnitRangesExt.jl
Outdated
Show resolved
Hide resolved
It's also possible TensorAlgebra.jl isn't the right place to define constructors like this, but I don't know where else they would be defined... |
Note that the package extension organization and names can be simplified once we change over GradedUnitRanges.jl to GradedArrays.jl. |
@lkdvos this is ready for another look, I'm curious what you think of the latest design. |
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 think this looks quite clean, it seems like you managed to decouple quite a lot of the dependencies this way
<:Any,<:Any,<:Any,<:Tuple{AbstractGradedUnitRange,Vararg{AbstractGradedUnitRange}} | ||
}, | ||
) | ||
# TODO: Define and use `blockdiagindices` |
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.
Something worth considering: in this function we are not guaranteed the user inputted a square matrix, in which case this function would still work, but spit out a random left- or right- isometry. I think if we call the function unitary
, we probably want to check for this, either by an explicit checksquare
, or some other means.
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.
Good point, I had that in mind but forgot to add that check. We should definitely check it is square (and more specifically, the blocks are the same and the codomain is the dual of the domain).
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.
The idea is definitely that it is unitary in a strict sense, in that it literally maps the space back to itself.
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.
After adding this check, I realized there is a crucial thing missing from this PR, which is specifying that the codomain and domain are duals of each other. That will require a separate PR to fusedims
to support that, though @ogauthe is reworking fusedims
/splitdims
to accommodate FusionTensors so I'll hold off on that for now.
Depends on ITensor/BlockSparseArrays.jl#85.
@lkdvos I've introduced a
qr_positive
constructor for matrices but we should probably have a tensor version too, related to the new factorization you introduced in #36.The motivation for this is defining a more general
"RandomUnitary"
gate in https://github.com/ITensor/QuantumOperatorDefinitions.jl that will handle symmetries.To-do:
Merge and register [WIP] DefineNot needed in the latest version.zeros
for graded unit ranges BlockSparseArrays.jl#85, or wait for moving that to a newGradedArrays.jl
package.