Fix dim for affine schemes (#2369) #2766

paemurru · 2023-09-04T15:55:19Z

Implemented dimension for localizations along

MPolyPowersOfElement,
MPolyComplementOfPrimeIdeal, and
MPolyComplementOfKPointIdeal

Not implemented for other localizations.
Enabled check for primeness in creating localized rings.
@HechtiDerLachs

Implemented dimension for localizations along complements of prime ideals and powers of an element. Not implemented for other localizations.

…ntIdeal

HechtiDerLachs

Thanks! I fixed some minor things, but overall, I think, this is good.

Treating both, the case of prime ideals and ideals of rational points with one signature is good in terms of avoiding code duplication. But on the other hand deciding primeness and dimension, etc. is much easier for the latter ideals so that writing code of its own for that case also seems worthwhile. I chose the solution via setting the necessary attributes to avoid unnecessary computation for the moment.

HechtiDerLachs · 2023-09-04T18:08:15Z

I guess the tests for primeness had been disabled due to @wdecker ? In my opinion it's OK to switch them on. I mean: We already saw in the tests for the MPolyQuos where not testing this leads to (see the fix).

afkafkafk13

Looks good to me. But to avoid duplicate code, the case by case distinction should happen on the algebra side, not on the geometry side. (see comment in the code)

afkafkafk13 · 2023-09-04T17:23:35Z

src/AlgebraicGeometry/Schemes/AffineSchemes/Objects/Attributes.jl

+  return dim(closure(X))
+end
+
+@attr function dim(X::AbsSpec{<:Ring, <:MPolyQuoLocRing{<:Any,<:Any,<:MPolyRing,<:MPolyRingElem, <:Union{MPolyComplementOfPrimeIdeal, MPolyComplementOfKPointIdeal}}})


Please do not do the two cases together. They differ significantly, which is otherwise hidden in the calls to other functions.
For MPolyComplementOfPrimeIdeal this version is fine.
In the case of MPolyComplementOfKPointIdeal, you do not need to adjust at all as dim(P) is guaranteed to be zero; you can just use "return dim(saturated_ideal(modulus(OO(X))))", which is the line you (correctly) removed above.

Yes. Now that I try to fix the case of MPolyComplementOfKPointIdeal over the integers, I also start to have my doubts whether putting the cases together is a good idea.

You write that dim(P) is guaranteed to be zero. Is it also zero over the integers?

afkafkafk13 · 2023-09-04T18:03:34Z

src/AlgebraicGeometry/Schemes/AffineSchemes/Objects/Attributes.jl

 @attr function dim(X::AbsSpec{<:Ring, <:MPolyQuoLocRing})
-  return dim(saturated_ideal(modulus(OO(X))))
+  error("Not implemented")
+end


One comment beforehand: Most of this should not happen on the geometric side, but on the algebra side. We need all these fixes on the algebra side as well and should only have them in one spot. The geometric side should directly refer dim (X) to dim(OO(X)) in the local and affine cases.

Ok, agreed. It seems that a lot of this is still missing on the algebra side, so this might be a good occasion to add it.

How serious are we about doing things on the algebra side?
If we are serious, then there would be only a single method:
dim(X::AbsSpec) = dim(OO(X))
and that is it. But yea, in the long run this is maybe the best way to go.

This is definitely the way to go. Otherwise there will be tons of duplicate code.

(My issue was meant as a marker not to forget this (large and necessary) cleanup. I wrote it on the geometric side because of my own lazyness, as I had stumbled into it on that side. But I always expected the solution to be implemented on the algebraic side and only a simple call on the geometric one.

afkafkafk13 · 2023-09-04T18:09:59Z

src/Rings/mpoly-localizations.jl


 Given a prime ideal ``P`` of a polynomial ring ``R``, say,
 return the multiplicatively closed subset ``R\setminus P.``

 !!! note
-    If  `check` is set to `true`, the function checks whether ``P`` is indeed a prime ideal. 
+    Since  `check` is set to `true`, the function checks whether ``P`` is indeed a prime ideal. 


This should still be an "if", as it can be overruled by input.

By default it is true, so I think that since fits.

HechtiDerLachs · 2023-09-04T18:33:02Z

src/Rings/mpoly-localizations.jl

+# parallel, we introduce the analogous method here. 
+function prime_ideal(S::MPolyComplementOfKPointIdeal) 
+  kk = coefficient_ring(ambient_ring(S))
+  # TODO: Test whether kk is an integral domain


I didn't find a way to test this. If anybody knows, let me know, too!

This is a primatlity test for the zero ideal of kk.

Yes, sure. I tried it. It fails over ZZ: The required methods are not there and we do not have a coherent interface. We need one method like is_integral_domain which we expect to be implemented for the ring.

I agree and I would explicitly test for the following cases:

field

integers
and not set is_maximal for the other cases.

Edit: This is of course for the lowest level. As long as kk=R[x] for some R, we can still go one level further down and decide there.

afkafkafk13 · 2023-09-04T18:35:19Z

src/Rings/mpoly-localizations.jl

  if !isdefined(S, :m)
    R = ambient_ring(S)
    x = gens(R)
    n = ngens(R)
    m = ideal(R, [x[i]-a for (i, a) in enumerate(point_coordinates(S))])
    set_attribute!(m, :is_prime=>true)
-    set_attribute!(m, :is_maximal=>true)
-    set_attribute!(m, :dim=>0)
+    set_attribute!(m, :is_maximal=>iszero(dim(kk)))


I doubt that this will work for kk = ZZ/<4>.

Yes. But nothing really does, so I think, it cannot be a real issue here. It wouldn't be mathematically incorrect here, would it?

This attribute is_maximal is mathematically incorrect here, as a maximal ideal in this example would be m+<2>, which strictly contains m.

codecov · 2023-09-04T20:55:09Z

Codecov Report

Merging #2766 (4c94180) into master (bfd0792) will increase coverage by 0.05%.
Report is 3 commits behind head on master.
The diff coverage is 83.78%.

@@            Coverage Diff             @@
##           master    #2766      +/-   ##
==========================================
+ Coverage   73.02%   73.08%   +0.05%     
==========================================
  Files         450      450              
  Lines       64160    64191      +31     
==========================================
+ Hits        46853    46914      +61     
+ Misses      17307    17277      -30

Files Changed	Coverage
src/Rings/mpoly-localizations.jl	`78.94%`
...ometry/Schemes/AffineSchemes/Objects/Attributes.jl	`85.71%`
src/Rings/mpoly-ideals.jl	`100.00%`

fingolfin · 2023-09-05T10:17:13Z

Hi @paemurru, thank you for your contribution. I assume the algebraic geometry people here know who you are, but right now your GitHub user profile is empty and so to an outsider you are a bit mysterious. In general we appreciate if we know for anyone who contributes at least the full name, ideally also affiliation. Perhaps you would be amenable to adding this to your GitHub user profile?

fingolfin · 2023-09-05T10:18:50Z

(I guess technically this would also be nice for @simonbrandhorst but there the GitHub username at least is the full name, and we know him, so it's not really a double standard...)

paemurru · 2023-09-05T12:33:01Z

your GitHub user profile is empty

Thanks, I added my full name and affiliation to my GitHub user profile.

simonbrandhorst

Looks good to me for now.
Moving everything to the commutative algebra side could be done later?

lgoettgens · 2023-09-05T13:34:11Z

Looks good to me for now. Moving everything to the commutative algebra side could be done later?

I have added #2771 to keep that in mind.

paemurru added 2 commits September 4, 2023 16:50

Fix dim for affine schemes (oscar-system#2639)

3ead045

Implemented dimension for localizations along complements of prime ideals and powers of an element. Not implemented for other localizations.

Implement dim for localizations with respect to MPolyComplementOfKPoi…

15e5de9

…ntIdeal

lgoettgens linked an issue Sep 4, 2023 that may be closed by this pull request

dim(X) gives wrong answer for X=Spec(A) and A localized at a prime ideal #2639

Closed

lgoettgens requested review from afkafkafk13 and HechtiDerLachs September 4, 2023 15:58

HechtiDerLachs added 4 commits September 4, 2023 18:36

Use caching and set attributes.

6295c95

Add some fixes for dimensions of ideals.

812695c

Add tests over the integers.

0988b5a

Fix tests for quotient rings.

0c32bc1

HechtiDerLachs approved these changes Sep 4, 2023

View reviewed changes

Introduce maximal_ideal.

3e1de68

afkafkafk13 reviewed Sep 4, 2023

View reviewed changes

HechtiDerLachs added 2 commits September 4, 2023 20:29

Fix localizations at K-points.

3bda73a

Add tests.

0ae9ff7

HechtiDerLachs reviewed Sep 4, 2023

View reviewed changes

afkafkafk13 reviewed Sep 4, 2023

View reviewed changes

HechtiDerLachs added 3 commits September 4, 2023 20:43

Add some tests over ZZ/4.

627de6f

Fix attributes.

bb16359

Fix tests.

4c94180

simonbrandhorst approved these changes Sep 5, 2023

View reviewed changes

simonbrandhorst merged commit 37eec92 into oscar-system:master Sep 5, 2023

lgoettgens mentioned this pull request Sep 5, 2023

Move dim implementation from schemes to algebras #2771

Open

paemurru deleted the ep/dim_fix branch September 7, 2023 11:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix dim for affine schemes (#2369) #2766

Fix dim for affine schemes (#2369) #2766

paemurru commented Sep 4, 2023 •

edited

Loading

HechtiDerLachs left a comment

HechtiDerLachs commented Sep 4, 2023

afkafkafk13 left a comment

afkafkafk13 Sep 4, 2023

HechtiDerLachs Sep 4, 2023

simonbrandhorst Sep 5, 2023

afkafkafk13 Sep 4, 2023

HechtiDerLachs Sep 4, 2023

simonbrandhorst Sep 4, 2023

afkafkafk13 Sep 4, 2023

afkafkafk13 Sep 4, 2023

simonbrandhorst Sep 5, 2023

HechtiDerLachs Sep 4, 2023

afkafkafk13 Sep 4, 2023

HechtiDerLachs Sep 4, 2023 •

edited

Loading

afkafkafk13 Sep 4, 2023 •

edited

Loading

afkafkafk13 Sep 4, 2023 •

edited

Loading

HechtiDerLachs Sep 4, 2023

afkafkafk13 Sep 4, 2023

codecov bot commented Sep 4, 2023

fingolfin commented Sep 5, 2023

fingolfin commented Sep 5, 2023

paemurru commented Sep 5, 2023

simonbrandhorst left a comment

lgoettgens commented Sep 5, 2023

Fix dim for affine schemes (#2369) #2766

Fix dim for affine schemes (#2369) #2766

Conversation

paemurru commented Sep 4, 2023 • edited Loading

HechtiDerLachs left a comment

Choose a reason for hiding this comment

HechtiDerLachs commented Sep 4, 2023

afkafkafk13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HechtiDerLachs Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

afkafkafk13 Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

afkafkafk13 Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 4, 2023

Codecov Report

fingolfin commented Sep 5, 2023

fingolfin commented Sep 5, 2023

paemurru commented Sep 5, 2023

simonbrandhorst left a comment

Choose a reason for hiding this comment

lgoettgens commented Sep 5, 2023

paemurru commented Sep 4, 2023 •

edited

Loading

HechtiDerLachs Sep 4, 2023 •

edited

Loading

afkafkafk13 Sep 4, 2023 •

edited

Loading

afkafkafk13 Sep 4, 2023 •

edited

Loading