Simple update refactoring#346
Conversation
Codecov Report❌ Patch coverage is
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
lkdvos
left a comment
lkdvos
left a comment
There was a problem hiding this comment.
Except for the comment about the boolean logic this looks good to me!
| return if bonddir == 1 # x-bond | ||
| rev ? rot180(x) : x | ||
| elseif bonddir == 2 # y-bond | ||
| rev ? (inv ? rotr90(x) : rotl90(x)) : (inv ? rotl90(x) : rotr90(x)) |
There was a problem hiding this comment.
This is a bit hard to read the logic behind this, am I correct that this is:
| rev\inv | false | true |
|---|---|---|
| false | R | L |
| true | L | R |
Or thus:
| rev ? (inv ? rotr90(x) : rotl90(x)) : (inv ? rotl90(x) : rotr90(x)) | |
| rev ⊻ inv ? rotl90(x) : rotr90(x) |
There was a problem hiding this comment.
I actually thought about the same thing. But what holds me back is that while rev and inv are clear individually, rev ⊻ inv does not appear to have a intuitive meaning.
revis whether the bond is reversed w.r.t. the canonical x/y direction (w - e or s - n).invis whether I'm rotating the bond to or from the original position.
So I'd rather write longer.
There was a problem hiding this comment.
I'm okay with that too, but then please just write the if statements, the triply nested ? on a single line is what tripped me up, I don't mind the if statments that much in this case, or even a combination of the two :)
There was a problem hiding this comment.
Fixed. Should look better now.
2 participants
Here are some refactorings I want to backport from my NTU branch.
_qr_bondnow can pass additionalkwargsfor finer control ofleft_orthinside. This is useful when the gate is applied before doingleft_orth, which needs an error-based truncation for rank-revealing._bond_rotationto do this._su_iter!signature.