-
Notifications
You must be signed in to change notification settings - Fork 126
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
New linear solving interface + more upstream changes #3350
Conversation
cf21ff8
to
79d9755
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #3350 +/- ##
==========================================
- Coverage 82.08% 82.06% -0.03%
==========================================
Files 556 556
Lines 74072 74075 +3
==========================================
- Hits 60805 60792 -13
- Misses 13267 13283 +16
|
711619b
to
f4d9332
Compare
1a6849f
to
6c10257
Compare
@@ -75,7 +75,7 @@ end | |||
=# | |||
|
|||
function rank(phi::FreeModuleHom{FreeMod{T}, FreeMod{T}, Nothing}) where {T<:FieldElem} | |||
return ngens(domain(phi)) - left_kernel(sparse_matrix(phi))[1] | |||
return ngens(domain(phi)) - nrows(kernel(sparse_matrix(phi), side = :left)) |
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.
I think we need a function nullity - to return the rank of the kernel only...
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.
I add it to the list of future improvements.
13052aa
to
d0af366
Compare
There is a type inference problem on nightly. It already happens only with Hecke (but not only with Nemo):
@thofma can/should we do something about this? |
Regarding the new invalidation: open an issue with the details (we can look at it with help of JET at some point), but otherwise move on. |
Not clear if this is about invalidations. julia> @inferred gens(Kx)
ERROR: return type Vector{AbstractAlgebra.Generic.MPoly{AbsSimpleNumFieldElem}} does not match inferred return type AbstractVector{<:AbstractAlgebra.Generic.MPoly{AbsSimpleNumFieldElem}}
Stacktrace:
[1] error(s::String)
@ Base ./error.jl:35
[2] top-level scope
@ REPL[11]:1
julia> @code_warntype gens(Kx)
[...]
Body::Vector{AbstractAlgebra.Generic.MPoly{AbsSimpleNumFieldElem}}
[...] This is inconsistent and one of them is lying. |
There is something dodgy in AA, where we rely on some compiler heuristic. Not sure what exactly broke it. I think I have a fix in Nemocas/AbstractAlgebra.jl#1611. Can you confirm that this fixes the issue @joschmitt? |
Yes, this also solves the original failure in the nightly tests here. |
This should get backported, right? |
This should be backported. It includes two breaking changes to the linear algebra, which are long overdue. Clean up I adjusted the book two weeks ago for the indexing changes (#3276) is the corresponding OSCAR PR) and the book does not use CC: @fieker |
* feat: Use new linear algebra functionality * Adjust to deleted deprecations/aliases Hecke edition
### Backported PRs - [x] #3367 - [x] #3379 - [x] #3369 - [x] #3291 - [x] #3325 - [x] #3350 - [x] #3351 - [x] #3365 - [x] #3366 - [x] #3382 - [x] #3373 - [x] #3341 - [x] #3346 - [x] #3381 - [x] #3385 - [x] #3387 - [x] #3398 - [x] #3399 - [x] #3374 - [x] #3406 - [x] #2823 - [x] #3298 - [x] #3386 - [x] #3412 - [x] #3392 - [x] #3415 - [x] #3394 - [x] #3391
The functions
solve
,can_solve
,can_solve_with_solution
,can_solve_with_solution_and_kernel
andkernel
(for matrices) now use the "new" interface from AbstractAlgebra.This is a breaking change
and needs a Hecke update.I put everything on top of #3366 so that OSCAR works with the newest Nemo/AbstractAlgebra. Will rebase once #3366 is merged.Closes #1062 🥳 🎉