Refactor quadratic form #64
Conversation
| if basis(A) isa DiracBasis | ||
| return c == cfs1 | ||
| else | ||
| dc = coeffs(c, basis(a), DiracBasis(object(parent(a)))) |
There was a problem hiding this comment.
@kalmarek You need to explain me what this was all about
There was a problem hiding this comment.
it's a stopgap solution on how to construct SparseCoefficients for a unit in arbitrary basis.
Here what I try to do is to create the unit in the DiracBasis which does contain unit as a vector and then rewrite it to the basis of a.
improvements are welcome!
However I think what you suggested is wrong:
if !isone(length(nonzero_pairs(c)))
return false
end
key, coef = only(nonzero_pairs(c))
return isone(coef) && isone(basis(a)[key])The suggestion is correct only when unit is an element of a basis. If our basis contains vectors (1+x, 1-x) (add 1/√2 factor to make it orthogonal) then the unit will be written with the corresponding coefficients (1/2, 1/2) (add √2 if you like).
| MA.operate!( | ||
| UnsafeAdd(), | ||
| res, | ||
| SparseCoefficients((_key(op.structure, k),), (cfs,)), |
There was a problem hiding this comment.
This _key things seems hacky, can't you apply _key in the structure function just before returning ? So that this complexity does not leave the multiplicative struct ?
There was a problem hiding this comment.
well, according to git blame You added this _key thing to work around the MTable quirkiness ;)
But anyway, maybe I need to rethink MTable as well (or disable it completely for now, until we get the things working?)
| if basis(A) isa DiracBasis | ||
| return c == cfs1 | ||
| else | ||
| dc = coeffs(c, basis(a), DiracBasis(object(parent(a)))) |
There was a problem hiding this comment.
it's a stopgap solution on how to construct SparseCoefficients for a unit in arbitrary basis.
Here what I try to do is to create the unit in the DiracBasis which does contain unit as a vector and then rewrite it to the basis of a.
improvements are welcome!
However I think what you suggested is wrong:
if !isone(length(nonzero_pairs(c)))
return false
end
key, coef = only(nonzero_pairs(c))
return isone(coef) && isone(basis(a)[key])The suggestion is correct only when unit is an element of a basis. If our basis contains vectors (1+x, 1-x) (add 1/√2 factor to make it orthogonal) then the unit will be written with the corresponding coefficients (1/2, 1/2) (add √2 if you like).
| MA.operate!( | ||
| UnsafeAdd(), | ||
| res, | ||
| SparseCoefficients((_key(op.structure, k),), (cfs,)), |
There was a problem hiding this comment.
well, according to git blame You added this _key thing to work around the MTable quirkiness ;)
But anyway, maybe I need to rethink MTable as well (or disable it completely for now, until we get the things working?)
| function MA.operate!(op::UnsafeAddMul, res, A, B, α) | ||
| for (kA, vA) in nonzero_pairs(A) | ||
| for (kB, vB) in nonzero_pairs(B) | ||
| cfs = MA.@rewrite α * vA * vB | ||
| MA.operate!(op, res, op.structure(kA, kB), C, cfs) | ||
| MA.operate!(op, res, op.structure(kA, kB), α * vA * vB) |
There was a problem hiding this comment.
I understand that you just removed the ABCα variant of UnsafeAddMul? Can you also remove the failing tests (as we don't plan to support this feature)?
|
Note that I think this PR made SymbolicWedderburn fail 😢 |
|
yes, since it relies on MTables |
No description provided.