-
Notifications
You must be signed in to change notification settings - Fork 65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add mutating arithmetic for SRow
s
#1659
base: master
Are you sure you want to change the base?
Conversation
Is the failure something known that sometimes crashes? Error in testset ModAlgAss/Lattices/Lattices.jl:
Error During Test at /home/runner/work/Hecke.jl/Hecke.jl/test/testdefs.jl:20
Got exception outside of a @test
LoadError: Matrix must be square
Stacktrace:
[1] error(s::String)
@ Base ./error.jl:35
[2] det
@ ~/work/Hecke.jl/Hecke.jl/src/LinearAlgebra/FakeFmpqMat.jl:357 [inlined]
[3] _coprime_integral_ideal_class(a::Hecke.AlgAssAbsOrdIdl{MatAlgebra{QQFieldElem, QQMatrix}, MatAlgebraElem{QQFieldElem, QQMatrix}}, b::Hecke.AlgAssAbsOrdIdl{MatAlgebra{QQFieldElem, QQMatrix}, MatAlgebraElem{QQFieldElem, QQMatrix}})
@ Hecke ~/work/Hecke.jl/Hecke.jl/src/AlgAssAbsOrd/PicardGroup.jl:922
[4] _is_principal_non_maximal(I::Hecke.AlgAssAbsOrdIdl{MatAlgebra{QQFieldElem, QQMatrix}, MatAlgebraElem{QQFieldElem, QQMatrix}})
@ Hecke ~/work/Hecke.jl/Hecke.jl/src/NumFieldOrd/NfOrd/PicardGroup.jl:363
[5] _is_principal_with_data_etale(a::Hecke.AlgAssAbsOrdIdl{MatAlgebra{QQFieldElem, QQMatrix}, MatAlgebraElem{QQFieldElem, QQMatrix}}; local_freeness::Bool)
@ Hecke ~/work/Hecke.jl/Hecke.jl/src/AlgAssAbsOrd/PicardGroup.jl:488
[6] _is_principal_with_data_etale
@ ~/work/Hecke.jl/Hecke.jl/src/AlgAssAbsOrd/PicardGroup.jl:478 [inlined]
[7] #_is_principal_with_data#4748
@ ~/work/Hecke.jl/Hecke.jl/src/AlgAssAbsOrd/PIP.jl:44 [inlined]
[8] _is_isomorphic_with_isomorphism_same_ambient_module(L::Hecke.ModAlgAssLat{Hecke.AlgAssAbsOrd{GroupAlgebra{QQFieldElem, MultTableGroup, MultTableGroupElem}, GroupAlgebraElem{QQFieldElem, GroupAlgebra{QQFieldElem, MultTableGroup, MultTableGroupElem}}}, Hecke.ModAlgAss{QQField, QQMatrix, GroupAlgebra{QQFieldElem, MultTableGroup, MultTableGroupElem}}, QQMatrix}, M::Hecke.ModAlgAssLat{Hecke.AlgAssAbsOrd{GroupAlgebra{QQFieldElem, MultTableGroup, MultTableGroupElem}, GroupAlgebraElem{QQFieldElem, GroupAlgebra{QQFieldElem, MultTableGroup, MultTableGroupElem}}}, Hecke.ModAlgAss{QQField, QQMatrix, GroupAlgebra{QQFieldElem, MultTableGroup, MultTableGroupElem}}, QQMatrix}, ::Val{true})
@ Hecke ~/work/Hecke.jl/Hecke.jl/src/ModAlgAss/Lattices/Morphisms.jl:289
[9] _is_isomorphic(L::Hecke.ModAlgAssLat{Hecke.AlgAssAbsOrd{GroupAlgebra{QQFieldElem, MultTableGroup, MultTableGroupElem}, GroupAlgebraElem{QQFieldElem, GroupAlgebra{QQFieldElem, MultTableGroup, MultTableGroupElem}}}, Hecke.ModAlgAss{QQField, QQMatrix, GroupAlgebra{QQFieldElem, MultTableGroup, MultTableGroupElem}}, QQMatrix}, M::Hecke.ModAlgAssLat{Hecke.AlgAssAbsOrd{GroupAlgebra{QQFieldElem, MultTableGroup, MultTableGroupElem}, GroupAlgebraElem{QQFieldElem, GroupAlgebra{QQFieldElem, MultTableGroup, MultTableGroupElem}}}, Hecke.ModAlgAss{QQField, QQMatrix, GroupAlgebra{QQFieldElem, MultTableGroup, MultTableGroupElem}}, QQMatrix}, with_iso_val::Val{true})
@ Hecke ~/work/Hecke.jl/Hecke.jl/src/ModAlgAss/Lattices/Morphisms.jl:[334](https://github.com/thofma/Hecke.jl/actions/runs/11477268995/job/31938971542?pr=1659#step:7:337)
[10] is_isomorphic_with_isomorphism(L::Hecke.ModAlgAssLat{Hecke.AlgAssAbsOrd{GroupAlgebra{QQFieldElem, MultTableGroup, MultTableGroupElem}, GroupAlgebraElem{QQFieldElem, GroupAlgebra{QQFieldElem, MultTableGroup, MultTableGroupElem}}}, Hecke.ModAlgAss{QQField, QQMatrix, GroupAlgebra{QQFieldElem, MultTableGroup, MultTableGroupElem}}, QQMatrix}, M::Hecke.ModAlgAssLat{Hecke.AlgAssAbsOrd{GroupAlgebra{QQFieldElem, MultTableGroup, MultTableGroupElem}, GroupAlgebraElem{QQFieldElem, GroupAlgebra{QQFieldElem, MultTableGroup, MultTableGroupElem}}}, Hecke.ModAlgAss{QQField, QQMatrix, GroupAlgebra{QQFieldElem, MultTableGroup, MultTableGroupElem}}, QQMatrix})
@ Hecke ~/work/Hecke.jl/Hecke.jl/src/ModAlgAss/Lattices/Morphisms.jl:303
[11] is_free_with_basis(L::Hecke.ModAlgAssLat{Hecke.AlgAssAbsOrd{GroupAlgebra{QQFieldElem, MultTableGroup, MultTableGroupElem}, GroupAlgebraElem{QQFieldElem, GroupAlgebra{QQFieldElem, MultTableGroup, MultTableGroupElem}}}, Hecke.ModAlgAss{QQField, QQMatrix, GroupAlgebra{QQFieldElem, MultTableGroup, MultTableGroupElem}}, QQMatrix})
@ Hecke ~/work/Hecke.jl/Hecke.jl/src/ModAlgAss/Lattices/Morphisms.jl:394
[12] top-level scope
@ ~/work/Hecke.jl/Hecke.jl/test/ModAlgAss/Lattices/Lattices.jl:5 |
Looks good to me, thanks |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1659 +/- ##
==========================================
+ Coverage 75.84% 75.88% +0.03%
==========================================
Files 361 361
Lines 113694 113865 +171
==========================================
+ Hits 86234 86409 +175
+ Misses 27460 27456 -4
|
needs a rebase as #1658 has been merged P.S.: Have not seen this error before. Trying to reproduce it locally. Should be unrelated to the changes here. |
b2058da
to
9ddf26e
Compare
i.e. don't throw on zero scalars, and coerce scalars if needed
typo in title: arithmetcs |
@@ -465,20 +467,23 @@ function scale_row!(a::SRow{T}, b::T) where T | |||
deleteat!(a.values, i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Orthogonal to this PR, but: Since you now handle b==0
at the start, the iszero(a.values[i])
check above the fold can only return true
if the coefficient ring is not a domain. So it could be strengthened to something like !is_domain_type(T) && iszero(a.values[i])
.
Since is_domain_type
is a trait depending only on T
, the compiler can eliminate the is_domain_type(T)
check -- if it returns true
it can elide the if
block, and if it is false
we the same code we have currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this can easily wait for a follow-up PR. (Also scale_row!
and scale_row_right!
could be merged)
SRow
sSRow
s
SRow
sSRow
s
What is planned with respect to tests? |
We have some generic functions in AA that test if the mutating ops are properly implemented. But as I wrote above, it is not that easy to apply them here, but I do my best to somehow put that together |
I was not sure whether this comment means that we want to
|
Wait with merging until I have time to look into this on Monday |
f2d4633
to
c5fb659
Compare
I added some tests regarding The two options now (@thofma needs to decide between):
|
Let's wait for 2) |
Add mutating arithmetics for SRow with the interface from AA. Inspired by oscar-system/Oscar.jl#4228 (comment) by @fingolfin .
Furthermore, make some of the existing functions easier to use by letting them coerce scalars (if needed) and add automatic workarounds for assertions.
I tested the new methods locally using
test_mutating_op_like_*
from AA. Unfortunately, for CI this does not work atm as there is too much expected missing for SRows (like random elements,zero
, ...).This currently contains #1658 to avoid conflicts.
@fingolfin could you have a look at the methods added here as well?