adding Commutators, Anticommutator, Daggers #40
Conversation
|
I left a couple of minor comments, nothing to interesting, and you might have already been thinking about some of these things. I will convert this to a draft. When it is ready for review and it has all tests pass, just convert it back and request a review. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #40 +/- ##
==========================================
+ Coverage 54.70% 62.78% +8.07%
==========================================
Files 14 15 +1
Lines 446 575 +129
==========================================
+ Hits 244 361 +117
- Misses 202 214 +12 ☔ View full report in Codecov by Sentry. |
|
I updated functionalities of (anti)commutator and dagger. Also added operator properties (hermitian, unitary, and commutative) and wrote corresponding simplification rules, which are all tested. I added doctests as well for basic operations. |
There was a problem hiding this comment.
This is a very solid start and I am very happy to see the momentum you already have!
I have a ton of questions for now, that will inform me how to understand this work. Let's go through them and then we can do another round of review and changes.
Converting back to draft -- convert to nondraft and request a review when you feel it is appropriate.
| if isexpr(x) | ||
| if operation(x)==operation(y) | ||
| ax,ay = arguments(x),arguments(y) | ||
| (length(ax) == length(ay)) && all(x -> _in(x, y), ax) |
There was a problem hiding this comment.
This should probably be just Set(ax)==Set(ay) -- should be asymptotically faster too, although I am not sure it matters for the sizes of expressions we would typically work with.
Having canonical order of arguments is also a good idea as you had already mentioned. I believe it is a better solution than what you currently have. But it is indeed not necessary for now.
However, to clean up some code repetition, I think I would prefer if this method is deleted and the already existing method is modified to basically say:
if operation(x) === :+
Set(ax) == Set(ay)
else
(length(ax) == length(ay)) && all(zip(ax,ay)) do xy isequal(xy...) end
end
There was a problem hiding this comment.
Set doesn't appear to work here, as equality of sets relies on == rather than isequal function. Here's an example:
julia> A = SOperator(:A, SpinBasis(1//2)); B = SOperator(:B, SpinBasis(1//2));
julia> Set([A+B]) == Set([A+B])
false
julia> isequal(A+B, A+B)
true
julia> A+B == A+B
false
There was a problem hiding this comment.
I defined set containment of arguments of expressions containing quantum objects with _in(x::SymQObj, y::SymQObj). Performance-wise, it's probably not better, but unless we want to redefine == for quantum objects, it might be better to stick with this option.
src/QSymbolicsBase/predefined.jl
Outdated
| dagger(x::Symbolic{AbstractBra}) = SDagger{AbstractKet}(x) | ||
| dagger(x::Symbolic{AbstractKet}) = SDagger{AbstractBra}(x) | ||
| dagger(x::Symbolic{AbstractOperator}) = SDagger{AbstractOperator}(x) | ||
| dagger(x::SKet) = SBra(x.name, x.basis) |
There was a problem hiding this comment.
I think I disagree with this one. You are tinkering beyond the domain of expression trees by doing this operation, while symbolics is usually only about the expression trees. This is a precedent that can make future work more difficult because now expression manipulation logic can not be simply encoded as tree-traversal.
In other words, there is no convenient way to say that SKet(:x) and SBra(:x) are related in the symbolic language we have here, because the pattern matching rules can not peek inside of a leaf term in the expression tree.
This is also related to the commented out test.
There was a problem hiding this comment.
OK, suppose we have a symbolic ket, say |a⟩, and we want to apply dagger to this ket. So instead of following the definition that dagger(|a⟩) == ⟨a|, as done in this PR, we should leave it as dagger(|a⟩) == |a⟩†? And this is because we are kind of cheating with the former approach, rather than traversing the expression tree? I'm not disagreeing with you, I'm just making sure I'm understanding what you're saying.
There was a problem hiding this comment.
Yup, that is it. There is no guarantee that |a⟩ and ⟨a| are related at all (except for you implicitly deciding so). To make it explicit you need to write some pattern matching rule that makes it evident that they are related. Such a pattern matching rule would be difficult in a setup that is not based on the expressions API. By just creating a new expression type dagger, that problem of implicit relationships disappears -- everything becomes explicit.
There was a problem hiding this comment.
Got it, just made changes.
|
Woohoo! |
|
Excellent! 😁 |
Defined constructors for bra, product of operators, commutator, anti commutator, and dagger. Also added scaling and a few simplification rules.