move code from Oscar.jl/experimental/GModule/Misc.jl here
#1675
Conversation
The idea is to add this code to AbstractAlgebra.jl, and then to remove it from Oscar.jl. Once we agree what shall be really added here, tests and documentation has to be added. (I am quite sure that I have put certain parts into the wrong files, and that some functions have to be changed.)
|
We need to adjust the signature here at least a little bit to not have something like oscar-system/Oscar.jl#3576 again. |
|
Yes, I wonder how we plan to do this? This is breaking for Oscar 1.0 and master. We cannot move methods around in a backwards compatible way. |
We can always put something like this into a "technically breaking" release of AA/Nemo/Hecke etc. as we plan to do with the printing changes anyway. Oscar 1.0 will never know about this, Oscar master just needs to get these functions removed in the same commit the AA compat is adapted. |
|
We are accumulating a bunch of "technically breaking" changes right now, so let's just label this as |
|
On Wed, Apr 17, 2024 at 08:17:14AM -0700, Tommy Hofmann wrote:
Yes, I wonder how we plan to do this? This is breaking for Oscar 1.0 and master. We cannot move methods around in a backwards compatible way.
I'd say it depends on the definition. At the end of the move, nothing
has changed (to the user)...
OK, in AA, Nemo, Hecke there might be aditional functions, but in Oscar
all that was there before is there, just the location of the
implementation has changed
… --
Reply to this email directly or view it on GitHub:
#1675 (comment)
You are receiving this because you are subscribed to this thread.
Message ID: ***@***.***>
|
I do not understand what is happening here:
Installing a `matrix(phi::IdentityMap{<:AbstractAlgebra.FPModule})` method
seems to make another `matrix` method unavailable?
|
The tests failed. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1675 +/- ##
==========================================
- Coverage 86.88% 86.39% -0.49%
==========================================
Files 116 116
Lines 29652 29643 -9
==========================================
- Hits 25764 25611 -153
- Misses 3888 4032 +144 ☔ View full report in Codecov by Sentry. |
|
Just to explain why this is "technically" breaking: because this will cause warnings in OSCAR of the kind "warning, method redefined in file X line Y, was previously defined in AbstractAlgebra; this may break precompilation". And then break CI tests, even though the methods we add here are identical to the ones in OSCAR. But Julia does not know this and panics. |
|
@ThomasBreuer what was the error you saw but did not understand? Perhaps if you can quote it here somebody has an idea? |
src/generic/DirectSum.jl
Outdated
| @@ -20,6 +20,10 @@ base_ring(N::DirectSumModule{T}) where T <: RingElement = base_ring(N.m[1]) | |||
|
|
|||
| base_ring(v::DirectSumModuleElem{T}) where T <: RingElement = base_ring(v.parent) | |||
|
|
|||
| is_free(M::DirectSumModule) = all(is_free, M.m) | |||
There was a problem hiding this comment.
Actually, isn't this mathematically wrong? That is: if all summands are free, then M is free. But M might be free even though its summands are not (keyword: projective modules, etc.)
There was a problem hiding this comment.
@fieker In which situation was is_free(::DirectSumModule) missing?
There was a problem hiding this comment.
@thofma points out:
If it is only applied to finitely generated/presented modules over fields or PIDs, then the method is correct:
$M \oplus N$ is free if and only if$M$ and$N$ are free.
This is of course true, but is there a way we can use this knowledge to improve the code above? I mean we could certainly restrict it to modules over fields; or over AA "integers", and AA univariates polynomials over fields, and perhaps a few more. But we can't do this generally here (e.g. not for fields over Nemo.ZZRing). But maybe we don't, it depends on what the application here is?
There was a problem hiding this comment.
No, but it is the same for all the functionality of things involving FPModule.
There was a problem hiding this comment.
Aha. The documentation indeed says:
# Free Modules and Vector Spaces
AbstractAlgebra allows the construction of free modules of any rank over any
Euclidean ring and the vector space of any dimension over a field.
but that's the only place in docs/src/free_module.md mentioning Euclidean
and it then proceeds to say
AbstractAlgebra provides generic types for free modules and vector spaces,
via the type `FreeModule{T}` for free modules, where `T`
is the type of the elements of the ring $R$ over which the module is built.
which to makes it not at all clear whether or not e.g. FreeModule{T} is only supposed to be used over Euclidean rings.
And docs/src/direct_sum.md does not mention Euclidean/principal/PID at all.
But docs/src/module.md then goes on to say this
The generic code provided by AbstractAlgebra will only work for modules over
euclidean domains.
which ought to settle it, except it then proceeds in the very next sentence to say
Free modules can be built over both commutative and noncommutative rings. Other
types of module are restricted to fields and euclidean rings.
which again leaves it completely unclear to me what is supposed to work in which case or not.
I think adding an is_free method that knowingly returns wrong results sometimes is problematic. At the very least its docstring then should contain a big warning?
Better would be if we could just restrict it -- e.g. if this only for direct sums over integers or fields it'd be fine. But that begs the question where is even used -- @fieker?
Alternatively I could also live with such a method:
function is_free(M::DirectSumModule)
f = all(is_free, M.m)
@req f || is_euclidean(base_ring(M)) "is_free for direct sums over non-euclidean domains not supported"
return f
end
is_euclidean(::Field) = true
is_euclidean(::ZZRing) = true
is_euclidean(::PolyRing{<:Field}) = true
is_euclidean(::Ring) = error("cannot tell if this ring is Euclidean")
src/generic/DirectSum.jl
Outdated
| @@ -220,6 +224,11 @@ function direct_sum(m::Vector{<:AbstractAlgebra.FPModule{T}}) where T <: RingEle | |||
| return M, inj, pro | |||
| end | |||
|
|
|||
| function direct_sum(M::DirectSumModule{T}, N::DirectSumModule{T}, mp::Vector{ModuleHomomorphism{T}}) where T | |||
There was a problem hiding this comment.
this function should have hom in its name. In Oscar, we call the similar function that lifts homs to a tensor product hom_tensor. For direct sums, there is the equivalent for Lie algebra modules as hom_direct_sum, but we can adapt the name of that in Oscar to match this one here. But IMO it should start with hom_*
When the |
|
On Thu, Apr 18, 2024 at 07:20:35AM -0700, Lars Göttgens wrote:
@lgoettgens commented on this pull request.
> @@ -220,6 +224,11 @@ function direct_sum(m::Vector{<:AbstractAlgebra.FPModule{T}}) where T <: RingEle
return M, inj, pro
end
+function direct_sum(M::DirectSumModule{T}, N::DirectSumModule{T}, mp::Vector{ModuleHomomorphism{T}}) where T
this function should have `hom` in its name. In Oscar, we call the similar function that lifts homs to a tensor product `hom_tensor`. For direct sums, there is the equivalent for Lie algebra modules as `hom_direct_sum`, but we can adapt the name of that in Oscar to match this one here. But IMO it should start with `hom_*`
I am happy to discuss this, but in Oscar, originally it was even just
called sum. As with all this things, they started with abelian groups in
Hecke...
But I'd be happy to dicuss names. We have
- a matrix of homs from Mi to Nj
- a vector from Mi to Ni
- a vector of homs from M to Ni
So many functions, so little names...
…
--
Reply to this email directly or view it on GitHub:
#1675 (review)
You are receiving this because you commented.
Message ID: ***@***.***>
|
|
On Thu, Apr 18, 2024 at 04:30:37PM +0200, ***@***.*** wrote:
On Thu, Apr 18, 2024 at 07:20:35AM -0700, Lars Göttgens wrote:
> @lgoettgens commented on this pull request.
>
>
>
> > @@ -220,6 +224,11 @@ function direct_sum(m::Vector{<:AbstractAlgebra.FPModule{T}}) where T <: RingEle
> return M, inj, pro
> end
>
> +function direct_sum(M::DirectSumModule{T}, N::DirectSumModule{T}, mp::Vector{ModuleHomomorphism{T}}) where T
>
> this function should have `hom` in its name. In Oscar, we call the similar function that lifts homs to a tensor product `hom_tensor`. For direct sums, there is the equivalent for Lie algebra modules as `hom_direct_sum`, but we can adapt the name of that in Oscar to match this one here. But IMO it should start with `hom_*`
I am happy to discuss this, but in Oscar, originally it was even just
called sum. As with all this things, they started with abelian groups in
Sorry, it was called hom
… Hecke...
But I'd be happy to dicuss names. We have
- a matrix of homs from Mi to Nj
- a vector from Mi to Ni
- a vector of homs from M to Ni
So many functions, so little names...
>
> --
> Reply to this email directly or view it on GitHub:
> #1675 (review)
> You are receiving this because you commented.
>
> Message ID: ***@***.***>
|
Just |
O.k., I will rename it to |
|
On Fri, Apr 19, 2024 at 01:39:55AM -0700, Thomas Breuer wrote:
@ThomasBreuer commented on this pull request.
> @@ -20,6 +20,10 @@ base_ring(N::DirectSumModule{T}) where T <: RingElement = base_ring(N.m[1])
base_ring(v::DirectSumModuleElem{T}) where T <: RingElement = base_ring(v.parent)
+is_free(M::DirectSumModule) = all(is_free, M.m)
@fieker In which situation was `is_free(::DirectSumModule)` missing?
Can't tell anymore. It might also now be supported by a generic is_free
for FPModule over any field
The only tricky case would be for modules over Z where due to torsion
freeness is not automatic
… --
Reply to this email directly or view it on GitHub:
#1675 (comment)
You are receiving this because you were mentioned.
Message ID: ***@***.***>
|
|
If it is only applied to finitely generated/presented modules over fields or PIDs, then the method is correct: |
|
Maybe we can drop the |
|
|
||
| is_finite(M::FPModule{<:FinFieldElem}) = true | ||
|
|
||
| function is_sub_with_data(M::FPModule{T}, N::FPModule{T}) where T <: RingElement |
There was a problem hiding this comment.
maybe rename to is_sub_with_inclusion?
|
I've remove the |
* Bump dependencies * Remove moved aliases * Add `base_ring_type` methods * React to Nemocas/AbstractAlgebra.jl#1675 * React to Nemocas/Nemo.jl#1720 * Add QQBarField disambiguation method * Require AA 0.41.2 with `Generic.hom` fix * Add `base_ring(::QQAbField)` * Fix doctests * Fix booktests
The idea is to add this code to AbstractAlgebra.jl, and then to remove it from Oscar.jl.
Once we agree what shall be really added here, tests and documentation have to be added.
(I am quite sure that I have put certain parts into the wrong files, and that some functions have to be changed.)