Needed for SumOfSquares #34
Conversation
blegat
force-pushed
the
bl/sos
branch
3 times, most recently
from
acd2380 to
7128e8a
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #34 +/- ##
==========================================
- Coverage 85.85% 84.06% -1.80%
==========================================
Files 14 14
Lines 693 709 +16
==========================================
+ Hits 595 596 +1
- Misses 98 113 +15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
src/arithmetic.jl
Outdated
| args::Vararg{Any,N}, | ||
| ) where {N} | ||
| for arg in args | ||
| if arg isa AlgebraElement | ||
| @assert parent(res) == parent(arg) | ||
| end | ||
| end | ||
| mstr = mstructure(basis(res)) | ||
| MA.operate_to!(coeffs(res), mstr, _coeffs_if_element.(args)...) |
There was a problem hiding this comment.
I'm not a fan of _coeffs_if_element and this mechanic. Why is adding an AlgebraElement and e.g. some coefficients even supported? when is it useful?
There was a problem hiding this comment.
Maybe it's not needed. It's used here: https://github.com/jump-dev/SumOfSquares.jl/pull/355/files#diff-3ce98ebb7dc7f7313a7049ec96353c60cdc0cba45f9a9199255402a5dd2cc427R150-R158
What I can do is move the coefficient to one of the three algebra element so I think I can refactor so that I don't give values alone outside of an AlgebraElement so that we can assume that every argument is an AbstractCoefficients.
I'm close to get the SumOfSquares tests green, then I'll refactor and merge the open PRs here and there so that SumOfSquares work with the master versions of its dependencies and then I'll fix the symmetry reduction (it's possible to have the tests passing without this since it's only used in the doc tutorials ^^)
|
Tests are passing now for SumOfSquares locally so I think we're close to have what we need from StarAlgebras so I'll not open separate PRs for the changes that are needed, maybe some parts in this PR are not needed anymore so I'll see |
blegat
force-pushed
the
bl/sos
branch
7 times, most recently
from
bf123eb to
92bd54d
Compare
The purpose of this PR is not to be merged but to list the changes that are needed so that we can then cherry-pick them and then open separate PRs.
Variety of changes needed for SumOfSquares