-
Notifications
You must be signed in to change notification settings - Fork 130
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
Move dim
from schemes to rings
#4280
Move dim
from schemes to rings
#4280
Conversation
dim
from schemes to rings #2771dim
from schemes to rings
@attr Int function dim(R::MPolyQuoLocRing{<:Any, <:Any, <:MPolyRing, <:MPolyRingElem, <:Union{MPolyComplementOfPrimeIdeal, MPolyComplementOfKPointIdeal}}) | ||
P = prime_ideal(inverted_set(R)) | ||
I = saturated_ideal(modulus(R)) | ||
return dim(I) - dim(P) | ||
end |
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.
Please handle them separately. For MPolyComplementOfKPointIdeal P is certified to be zero-dimensional so that we do not need to compute dim(P).
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.
P is certified to be zero-dimensional
It seems that this is not true.
julia> R, (x, y) = ZZ[:x, :y]
(Multivariate polynomial ring in 2 variables over ZZ, ZZMPolyRingElem[x, y])
julia> S = complement_of_point_ideal(R, [0, 0])
Complement
of maximal ideal corresponding to rational point with coordinates (0, 0)
in multivariate polynomial ring in 2 variables over ZZ
julia> I = ideal(R, 0)
Ideal generated by
0
julia> L, _ = localization(R, S)
(Localization of multivariate polynomial ring in 2 variables over ZZ at complement of maximal ideal of point (0, 0), Hom: R -> localized ring)
julia> W, _ = quo(L, L(I))
(Localization of quotient of multivariate polynomial ring at complement of maximal ideal, hom: Localized ring -> Localized quotient of multivariate polynomial ring)
julia> P = prime_ideal(inverted_set(W))
Ideal generated by
x
y
julia> dim(P)
1
So I will leave it as it is for now.
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.
My guess is that complement_of_point_ideal
silently assumes that we are working over a field? Perhaps the type and constructor should be restricted accordingly what do you think @afkafkafk13 ?
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.
julia> S = complement_of_point_ideal(R, [0, 0])
Complement
of maximal ideal corresponding to rational point with coordinates (0, 0)
in multivariate polynomial ring in 2 variables over ZZ
This is indeed a problem with complement_of_point_ideal:
This ideal is NOT maximal as we do not get a field as the quotient of the ring by it. This needs to be handled in complement_of_point_ideal
. My suggestion: check dimension (not number of entries!!) of the ideal before doing anything else in complement_of_point_ideal
, throw an error if it is not maximal (i.e. dim(ideal) is not zero -- this should already be working in Oscar) and point the user to complement_of_prime_ideal
.
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.
It is a bit fuzzy what a "point" is here. Will we get anything computationally effective for the "point" given by the maximal ideal (2,x,y)
in Z[x,y]
? I impression was that ComplementOfKPointIdeal
just assumes a base field.
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 impression was that ComplementOfKPointIdeal just assumes a base field.
I think it does assume but does not check for this (although it should).
I would leave fixing complement_of_point_ideal
to a separate issue.
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.
ok. fine with me. But we urgently need to fix complement_of_point_ideal. Maybe we can just check
@simonbrandhorst no, we won't get anything computationally effective currently. You are right. We should just tell the user to use complement_of_prime_ideal
instead, whenever we are not over a field.
Edit: This is a strategic decision and breaks some tests. I opened PR We need to discuss this. Over a ring this is not a point ideal and hence should not be used via this localization, anyway. Moreover, the localization at <x_1,\dots,x_n> in ZZ[x_1,\dots,x_n] is not what is produced here, as we are localizing w.r.t. a monomial ordering and continue working over ZZ and do not pass to QQ, even though ZZ \cap <x_1,\dots,x_n> = 0.
src/Rings/mpolyquo-localizations.jl
Outdated
@attr Int function dim(R::MPolyLocRing{<:Any,<:Any,<:MPolyRing,<:MPolyRingElem, <:Union{MPolyComplementOfPrimeIdeal, MPolyComplementOfKPointIdeal}}) | ||
P = prime_ideal(inverted_set(R)) | ||
return codim(P) | ||
end |
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.
Again you might want to handle MPolyComplementOfKPointIdeal separately?
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 am not sure how to do this here.
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.
Thanks for looking into this.
Anne: "For MPolyComplementOfKPointIdeal P is certified to be zero-dimensional so that we do not need to compute dim(P)."
Co-authored-by: Simon Brandhorst <[email protected]>
(The bug was introduced and fixed in this pull request.)
Contributes to #2771 but does not close it because #2771 (comment)