-
Notifications
You must be signed in to change notification settings - Fork 112
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
Implement flattenings of graded polynomial rings #2906
Implement flattenings of graded polynomial rings #2906
Conversation
5e1706f
to
73a820a
Compare
U1 <: MPolyRingElem{T}, | ||
U2 <: MPolyRingElem{T}, # types in domain an codomain might differ: one might be decorated, the other not. | ||
DT <: Union{MPolyRing{T}, MPolyQuoRing{U1}}, | ||
CT <: Union{MPolyRing{T}, MPolyQuoRing{U2}}} |
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.
U1 <: MPolyRingElem{T}, | |
U2 <: MPolyRingElem{T}, # types in domain an codomain might differ: one might be decorated, the other not. | |
DT <: Union{MPolyRing{T}, MPolyQuoRing{U1}}, | |
CT <: Union{MPolyRing{T}, MPolyQuoRing{U2}}} | |
DT <: Union{MPolyRing{T}, MPolyQuoRing{<:MPolyRingElem{T}}}, | |
CT <: Union{MPolyRing{T}, MPolyQuoRing{<:MPolyRingElem{T}}}} |
For the case of different Us, there is no need to specify them concretely.
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.
Yes, seems reasonable. I just preferred to stay close to what I found from before, also to not confuse the original authors unnecessarily much in case they want to look and confirm. Either way is fine with me, but personally I wouldn't bother to change this.
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 left some comments and questions about the code and would be happy to discuss these at some point.
# TODO: implement further base changes. But for this we need more functionality | ||
# for potential change of grading groups. See the open pr #2677. | ||
function change_base_ring( |
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.
This actually became quite relevant for me in this PR: change_base_ring
for graded modules and translating the gradings. For special cases where I know the gradings don't change, you can customize this. But in general, I was forced to introduce an internal helper routine below. Maybe this can one day be used in a front-end method as a workhorse for the case of preserved gradings.
return MS, map | ||
end | ||
|
||
function _regularity_bound(M::SubquoModule) |
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 will need this for the algorithm to compute derived pushforwards which I described in my article here.
return F_flat, iso, iso_inv | ||
end | ||
|
||
function flatten( |
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 pursued the following approach: There is one user facing method flatten
that can be called on any reasonable object. The user does not need to care about which flattening is being used and where it is cached, etc. Internally, a RingFlattening
of R
is created and stored in the ring R
. This one is called by default in the flatten
methods and it maintains all associated flattened objects like ideals, modules, etc. The internal user/programmer can or rather should use a specified flattening and not the cached one when coding.
Does that make sense?
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.
In other words: flat_counterparts is some dictionary which encodes the correspondence between non-flattened and flattened objects -- to me the idea makes sense (provided this does not eat up too much space)
@@ -35,6 +37,30 @@ | |||
) | |||
end | |||
|
|||
function RingFlattening( |
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.
There's a lot of code duplication here for the graded cases. But I didn't find how to avoid this, really. Maybe the people involved in the development of graded stuff have some ideas?
if !haskey(flat_counterparts(phi), x) | ||
result = phi.orig_to_flat(x) | ||
flat_counterparts(phi)[x] = result | ||
return result | ||
end | ||
return flat_counterparts(phi)[x]::elem_type(codomain(phi)) |
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'm not 100% sure that doing this is really performant. And maybe we should also find a way to turn it off, e.g. using keyword arguments. Apart from that: @fingolfin , do you think this flat_counterpart
architecture that I set up here is a safe and correct application of the WeakKeyIdDict
s? I will link again to this below.
@@ -263,6 +411,30 @@ function map_from_coefficient_ring_to_flattening(phi::RingFlattening) | |||
return phi.base_ring_to_flat | |||
end | |||
|
|||
### flat counterparts of algebraic objects defined over the domain |
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.
Here I explain the philosophy of caching the flat_counterpart
s. One issue might be that everything with cached flat counterpart must be "mathematically immutable". I think that's more or less safe in Oscar, but one probably has to be careful with things like in-place operations on matrices. Does anyone see problems here?
@@ -3185,3 +3185,24 @@ end | |||
|
|||
dim(R::MPolyLocRing{<:Field, <:FieldElem, <:MPolyRing, <:MPolyElem, <:MPolyComplementOfPrimeIdeal}) = nvars(base_ring(R)) - dim(prime_ideal(inverted_set(R))) | |||
|
|||
######################################################################## | |||
# Localizations of graded rings # | |||
######################################################################## |
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 was happily surprised that localizing graded rings does not seem to pose any problems so far. The code is sufficiently generic so that it just works. But I am wondering whether there are some more functions which we should provide for such localizations, see below.
@@ -66,3 +66,12 @@ | |||
@test f(x) == x | |||
end | |||
end | |||
|
|||
@testset "cross-type kernels" begin |
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.
This is the test corresponding to my fix of the AffAlgHom
.
|
||
M, _ = quo(F, [u^5*F[1], v*F[1]]) | ||
M_flat, iso, iso_inv = Oscar.flatten(M) | ||
res = free_resolution(M) |
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.
@jankoboehm : Could you maybe have a look at this and tell me whether I implemented this custom free resolution in accordance with how you guys designed this? Maybe just run the code of this test in the REPL once, or something?
I will add a |
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.
Overall this looks good. I left you a few minor remarks in the code.
Just a remark on wording:
In commutative algebra 'flat' is of course a property of a module,
here you use it in the sense of 'flattened'. You might want to mention that somewhere in the file.
(By the way: I fully accept this choice for better readability of the code)
julia> homogeneous_coordinates_on_affine_cone(P) | ||
3-element Vector{MPolyQuoRingElem{QQMPolyRingElem}}: | ||
3-element Vector{MPolyQuoRingElem{MPolyDecRingElem{QQFieldElem, QQMPolyRingElem}}}: | ||
x |
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.
Not concerning this PR, but more generally: we need to rethink the printing here. The way it is now, it does not have any added value for the user, whereas it is a lot better in line 399.
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.
We cannot change the printing of julia Arrays.
return F_flat, iso, iso_inv | ||
end | ||
|
||
function flatten( |
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.
In other words: flat_counterparts is some dictionary which encodes the correspondence between non-flattened and flattened objects -- to me the idea makes sense (provided this does not eat up too much space)
R, (x, y) = polynomial_ring(QQ, [:x, :y], cached=false) | ||
S, (u, v) = grade(polynomial_ring(R, [:u, :v], cached=false)[1]) | ||
|
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.
Does this creation of S also work with graded_polynomial_ring?
If so, it might be appropriate to do this. (This is the case I expect)
If not, @fingolfin would certainly like to hear about the problem.
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.
Looks good to me.
julia> homogeneous_coordinates_on_affine_cone(P) | ||
3-element Vector{MPolyQuoRingElem{QQMPolyRingElem}}: | ||
3-element Vector{MPolyQuoRingElem{MPolyDecRingElem{QQFieldElem, QQMPolyRingElem}}}: | ||
x |
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.
We cannot change the printing of julia Arrays.
The difference to what we have is that we get graded rings again and they have the correct default ordering.