Updated operators to print with hat #79
Conversation
|
@Krastanov my main goal here was to familiarize myself with the pull-request process. I personally like my operators to display with hats, but I know this isn't a law, so feel free to discard this change. Any feedback on if I went through the pull request process would be welcome though :) |
|
This is great, Rohan, thanks! I will run a review on it later today |
There was a problem hiding this comment.
I love that the PR is small -- way easier to review and merge.
I left one inline comment on some code details.
There are a lot of failing tests. An easy way to run the tests locally and check things is to do ] test QuantumSymbolics -- that way you can find exactly which test is failing and focus on it.
Most of the tests here seem to be "doctests" that just lack a hat. There is an automated way to fix them (the fix argument to doctest function).
Happy to discuss testing more at the hackathon.
| @@ -138,7 +138,7 @@ basis(x::SymQ) = x.basis | |||
|
|
|||
| Base.show(io::IO, x::SKet) = print(io, "|$(symbollabel(x))⟩") | |||
| Base.show(io::IO, x::SBra) = print(io, "⟨$(symbollabel(x))|") | |||
| Base.show(io::IO, x::Union{SOperator,SHermitianOperator,SUnitaryOperator,SHermitianUnitaryOperator,SSuperOperator}) = print(io, "$(symbollabel(x))") | |||
| Base.show(io::IO, x::Union{SOperator,SHermitianOperator,SUnitaryOperator,SHermitianUnitaryOperator,SSuperOperator}) = print(io, "$(symbollabel(x))̂") | |||
There was a problem hiding this comment.
Two points:
-
we probably should insert the hat on the first letter (not all operators are a single symbol)
-
let's remove
SSuperOperatorfrom this list for the moment, as it has a bit more complicated printing
|
@Krastanov thanks for the feedback! I have updated the doctests. Hopefully they pass now. I actually added logic to make the hat print on the middle character instead. I know this wasn't your suggestion and it may be more logic that we are looking for here. Lmk what you think! |
|
Apologies, this dropped from my review queue and I forgot to get back to it. It should be reviewed sometime soon. |
|
apologies, it seems I messed up the merge with master. Let me try to fix that |
Krastanov
force-pushed
the
rbali_print
branch
from
a3aa526 to
499236f
Compare
|
Darn, there are a few new places in the documentation where the hats need to be added. For now I will mark this as a draft as I will not have the time to make these changes. Apologies for the delay. Happy to pick it back up if you are interested in finishing it, but no pressure on that. |
|
Hi @Krastanov , thanks for looking at it. Unfortunately I think I will need to drop this, atleast for now. |
|
no worries! |
Operators now print with hat on top