Spec retying #1537

HechtiDerLachs · 2022-09-13T11:21:32Z

This provides two major improvements for the schemes package:

Schemes can now in principal be set up for any type of ring. This was suggested by Ulrich Thiel and I finally figured out how to accomplish that in the given framework. As for now, we support polynomial rings, their quotients, and localizations of either two. This was made possible, since we now have almost coherent interfaces for the ideals in either case and for homomorphisms between either two of such types of rings (even though this still has to be streamlined and I had to make some dirty fixes).
Several objects now come with more clear interfaces and the possibility of inheritance. This makes way to derive more special schemes, such as group schemes, schemes of special dimension like curves and surfaces, toric schemes, etc., without the need for code duplication.

afkafkafk13 · 2022-09-14T19:47:40Z

docs/src/AlgebraicGeometry/AffineSchemes.md

@@ -6,56 +6,229 @@ CurrentModule = Oscar
 using Oscar
 ```

-# General schemes
+# General schemes and their interfaces

 Arbitrary schemes over a base ring ``\mathbb k`` which are given by means 


Don't you want to mention somewhere that every ring here should at least be commutative with unity?

I assumed this was understood. Has anyone ever seen schemes over non-commutative rings?

Ok, there are the non-commutative C*-algebras, but that is hardly ever called scheme.

Anyway: I think you are right. I will add that.

afkafkafk13 · 2022-09-14T20:20:24Z

experimental/Schemes/AffineSchemes.jl

-mult_set_type(::Type{Spec{BRT, BRET, RT, RET, MST}}) where {BRT, BRET, RT, RET, MST} = MST
-poly_ring_type(::Type{Spec{BRT, BRET, RT, RET, MST}}) where {BRT, BRET, RT, RET, MST} = RT
-poly_type(::Type{Spec{BRT, BRET, RT, RET, MST}}) where {BRT, BRET, RT, RET, MST} = RET
+function hypersurface_complement(X::SpecType, f::Vector{<:RingElem}) where {SpecType<:AbsSpec{<:Any, <:MPolyLocalizedRing}}


Why is f a vector here? For a hypersurface complement this should (from the point of view of mathematical theory) be a single element.

This is more for internal use and passing on factors of f whenever we already have it in partially factorized form.

afkafkafk13 · 2022-09-14T20:35:43Z

experimental/Schemes/AffineSchemes.jl

+  ) where {BRT}
+  R = base_ring(OO(X))
+  R == base_ring(OO(Y)) || error("schemes can not be compared")
+  return Spec(quo(OO(Y), OO(Y)(modulus(OO(X))))[1])


stupid question: don't you need to pay attention to the multiplicatively closed subsets used in the localizations of the two schemes (just as in the preceding instance)?

Oops. Yes, I was missing out on that.

Now that I'm looking at it again: No, only one of the rings has a multiplicative set and it's preserved, because I'm calling quo on OO(Y).

afkafkafk13 · 2022-09-15T05:47:17Z

experimental/Schemes/AffineSchemes.jl

+  end
+
+  function Spec(kk::Ring, R::Ring)
+    return new{typeof(kk), typeof(R)}(R)


Why is there only one argument here?

afkafkafk13 · 2022-09-15T07:38:45Z

experimental/Schemes/AffineSchemes.jl

@@ -455,34 +784,59 @@ function preimage(
  return Spec(MPolyQuoLocalizedRing(R, ideal(R, new_gens) + modulus(OO(X)), inverted_set(OO(X))*new_units))
 end

-function is_isomorphism(f::SpecMor)
+function preimage(f::AbsSpecMor, Z::AbsSpec{<:Ring, <:MPolyRing}; check::Bool=true)


I do not see Z used in the function anywhere. Is that, because Z is automatically the whole codomain here or do we at least need to check compatibility with the codomain of f (as a sanity check)?

Ok, this is a mistake.

Looking at it again: It's not a mistake. Since Z = Spec(P) for a polynomial ring, it must necessarily be the full ambient space of the codomain of f, so the preimage is simply a copy of the domain.

Let me put the question more directly: How do you make sure that the user has not provided some Z which has nothing to do with f at all (maybe by mistake or typo in the name or because a previous computation had an unexpected result)? As you are not touching Z anywhere, no sanity check (apart from the fact that Z is a Spec of some polynomial ring) is performed -- neither explicitly nor implicitly.

afkafkafk13

Significant rewrite, makes internal structure of the schemes-code a lot more readable and accesible to others.

Questions:

How did the performance change?
You have several open 'TODO' in there. Do you want to merge now and add those later on?

I see this pull request as aiming at setting up a stable foundation both for your further development/refinement of the scheme environment and for the ongoing development in packages relying on schemes. Thus I am in favor of approval.

To clarify why I am merely commenting instead of approving:

I have not checked explicitly that at all relevant instances all 16 pairs of possible underlying rings are handled properly (i.e. none of them escaped our attention)
I did not check that odd cases (like trying to map from or into an empty scheme) are caught.

afkafkafk13 · 2022-09-15T07:48:40Z

experimental/Schemes/AffineSchemes.jl

  return get_attribute(f, :inverse)::morphism_type(codomain(f), domain(f))
 end

 @Markdown.doc """
-    product(X::Spec, Y::Spec)
+    product(X::AbsSpec, Y::AbsSpec)

 Returns a triple ``(X×Y, p₁, p₂)`` consisting of the product ``X×Y`` and the two projections 
 ``p₁ : X×Y → X`` and ``p₂ : X×Y → Y``.


Do you want to mention that this is the product over the common base field k?

Yes, that should be mentioned and checked.

afkafkafk13 · 2022-09-15T08:06:01Z

experimental/Schemes/AffineSchemes.jl

+  return ngens(ambient_ring(X))
+end
+
+@attr function dim(X::AbsSpec{<:Ring, <:MPolyRing})


How are you making sure that you are not dealing with SpecZZ[x,y] ?

We already talked about this on the phone. Could you quickly document your suggestion here? What precisely is the oddity I have to take into account?

Don't you just want to leave this to the commutative algebra side of OSCAR and call something like the dimension of the zero ideal of your ring? All the shortcuts should reside on in the Rings or Ideals part, where this problem naturally appears and is certainly already taken care of.

Yes, sure. But I need to know what is the mathematical oddity that has to be addressed. I remember you said dim(Spec(ZZ[x,y]) == 3, because dim(ZZ) == 1. So what is the problem exactly and how do you suggest to resolve it? Should there be a function dim(R::MPolyRing) that we then use? Or should we do a manual case analysis of the base_ring?

Yes, this is precisely the oddity: If we are dealing with a polynomial ring over the integers in n variables, its dimension is n+1. If we are dealing with a quotient of a polynomial ring over the integers by some ideal, it makes a difference, whether the ideal contains an integer or not, but the backend Singular should take care of the oddity automatically as soon as it gets called.

OSCAR already has a function dim(Ideal) which returns the dimension of (Ring/Ideal), and we should use that applied to the ideal <0> in our ring here -- even if it feels odd. This shifts the case analysis to the commutative algebra, which in turn might or might not do it by itself or leave it to a backend like Singular -- in any case, the schemes-code are not the right place for the case analysis.

afkafkafk13 · 2022-09-15T10:19:41Z

experimental/Schemes/SpecOpen.jl

@@ -909,7 +980,14 @@ function restriction(
  return SpecOpenMor(U, V, new_maps_on_patches, check=check)
 end

-identity_map(U::SpecOpen) = SpecOpenMor(U, U, [SpecMor(V, ambient(U), gens(OO(V)), check=false) for V in affine_patches(U)], check=false)
+function identity_map(U::SpecOpen) 
+  [SpecMor(V, ambient(U), gens(OO(V)), check=false) for V in affine_patches(U)]


left-over of debugging?

afkafkafk13 · 2022-09-15T10:30:40Z

experimental/Schemes/SpecOpen.jl

@@ -1060,11 +1146,11 @@ function restriction_map(U::SpecOpen, X::Spec, h::MPolyElem; check::Bool=true)
  # first find some basic relation hᵏ= ∑ᵢ aᵢ⋅dᵢ
  d = gens(U)
  I = complement_ideal(U)
-  (k, poh) = Oscar._minimal_power_such_that(h, x->(x in I))
-  a = coordinates(poh, I)
+  (k, poh) = Oscar._minimal_power_such_that(h, x->(base_ring(I)(x) in I))


_minimal_power_such_that: I understand the name and the content of the function, but the name is not reaaly self explanatory

Yes, but this is an internal function so I did not bother to really explain it.

I was hoping it was self explanatory: _minimal_power_such_that(h, P) returns (k, h^k) where k is the minimal power of h such that property P(h^k) holds.

afkafkafk13 · 2022-09-15T10:37:29Z

experimental/Schemes/SpecOpen.jl

+  )
+  Y = ambient(U)
+  R = ambient_ring(Y)
+  R == ambient_ring(X) || error("rings not compatible")


small sideremark -- just to make sure not to forget it:
Your arguments are schemes, but your error message talks about rings. You could try to be more consistent in your wording (schemes not compatible/corresponding rings not compatible).

afkafkafk13 · 2022-09-15T10:54:00Z

experimental/Schemes/SpecialTypes.jl

+
+export SimpleGlueing
+
+@attributes mutable struct SimpleGlueing{LST<:AbsSpec, 


Would you mind adding a short description/documentation for that struct? 6 arguments are hard to keep track of, if you do not see the mathematical data behind it.

This is actually work in progress and not even tested, yet. I will add descriptions, eventually. However, this type will most probably not be user facing and the documentation should be made for the AbsGlueing interface.

HechtiDerLachs · 2022-09-15T11:35:12Z

Questions:

* How did the performance change?

* You have several open 'TODO' in there. Do you want to merge now and add those later on?

Which performance? I think we do not have any performance tests and nothing to compare it to. Either way: This will depend on the performance of computations in the rings used, so it's not part of this PR.

About the TODOs: Can you be more specific? I do not recall leaving any TODOs that I could have addressed at this point. But I have some that fall into the realm of others or have pending dependencies on others. So I guess whatever TODOs are still there, they are there for a reason, should be merged for the time being, and be addressed when the time is ripe. For instance coordinates for various types of ideals still has hacks but there's nothing I can do about it now.

afkafkafk13 · 2022-09-15T11:47:16Z

concerning performance: I was only wondering whether something might have struck you as significant while running test. This drastic change in the level of abstraction could have had sideeffects (in both directions....)

concerning TODO: I was mostly thinking of 'coordinates' and 'simplify' and my impression coincides with your statement.

HechtiDerLachs · 2022-09-15T11:55:39Z

concerning performance: I was only wondering whether something might have struck you as significant while running test. This drastic change in the level of abstraction could have had sideeffects (in both directions....)

I don't think that it is even reasonable at this point to start comparing performance. Simply because of the many inefficient hacks that are necessarily there for the time being. We will have to see again, once the new cached groebner bases are there and so on. This will stay work in progress and performance questions should be addressed by the people working on the respective rings/ideals, etc. I don't think it's part of the schemes, really.

But I did not see any significant decrease of speed in the tests, no.

afkafkafk13

There is precisely one question/point left to be addressed. See comment at the line

afkafkafk13 · 2022-09-16T07:04:36Z

experimental/Schemes/AffineSchemes.jl

-  return ngens(base_ring(OO(X)))-dim(X)
+# TODO: This is not 100% mathematically correct, since codimension is 
+# a local concept. Should we therefore forbid this method?
+@attr function codim(X::Spec)


You are actually aiming at height(modulus(X)), right? This involves a primary (or equidimensional) decomposition in the general, i.e. non-equidimensional, case and should be referred to the algebraic side, where all the functionality should be available.

Yes, height(modulus(OO(X)) would be one candidate. It depends what this function will be used for. This has probably been introduced to determine the expected rank of the jacobian matrix of some set of generators of modulus(OO(X)). In that case, invoking a primary decomposition first does not seem reasonable. So I would really vote for deleting this function and leave it to the user to exctract the information they need for their particular applications.

Why would you forbid this? It is well-defined globally and can be computed without primary decomposition, the hightest-dimensional component wins.

Yes, you are right, of course the highest-dimension component wins (Wald vor Bäumen nicht gesehen....).
This should just be
dimension of ambient scheme - dimension of X.

By the way: You do not get as much sensible information from the Jacobian criterion in the arithmetic case anyway...

afkafkafk13

Looks good.

Only one remark left to be discussed (maybe in direct communication).

afkafkafk13 · 2022-09-19T08:18:43Z

experimental/Schemes/AffineSchemes.jl

@@ -455,34 +784,59 @@ function preimage(
  return Spec(MPolyQuoLocalizedRing(R, ideal(R, new_gens) + modulus(OO(X)), inverted_set(OO(X))*new_units))
 end

-function is_isomorphism(f::SpecMor)
+function preimage(f::AbsSpecMor, Z::AbsSpec{<:Ring, <:MPolyRing}; check::Bool=true)


Let me put the question more directly: How do you make sure that the user has not provided some Z which has nothing to do with f at all (maybe by mistake or typo in the name or because a previous computation had an unexpected result)? As you are not touching Z anywhere, no sanity check (apart from the fact that Z is a Spec of some polynomial ring) is performed -- neither explicitly nor implicitly.

afkafkafk13

The most recent changes are ok and now all items are resolved.

HechtiDerLachs added 4 commits September 13, 2022 10:06

Port files from other branch.

b159533

Minor adjustments about inclusion of missing files.

7c3da81

Rewrite the documentation for affine schemes.

914f128

Fix documentation.

a956789

HechtiDerLachs mentioned this pull request Sep 14, 2022

ideal membership crashes for MPolyQuoIdeal in 'fake' quotient rings Q = R/(0) #1530

Closed

fingolfin requested review from afkafkafk13, jankoboehm and wdecker September 14, 2022 17:10

afkafkafk13 reviewed Sep 14, 2022

View reviewed changes

afkafkafk13 reviewed Sep 15, 2022

View reviewed changes

HechtiDerLachs added 2 commits September 15, 2022 16:18

Address various comments from GH discussion.

5a05e97

First steps towards toric schemes.

3260a29

afkafkafk13 reviewed Sep 16, 2022

View reviewed changes

HechtiDerLachs added 3 commits September 16, 2022 11:33

Add some comments [no-ci].

20ab1da

Updates from another branch.

981d04d

Fix tests and embrace SimpleGlueing.

f436e02

afkafkafk13 reviewed Sep 19, 2022

View reviewed changes

HechtiDerLachs added 5 commits September 19, 2022 23:17

Extract glueings from toric data.

365649d

Small changes after discussion.

d807cc8

Update from main branch.

9217e0b

Remove Toric schemes for the moment.

514f861

Add sanity check.

71a4e64

afkafkafk13 approved these changes Sep 20, 2022

View reviewed changes

fingolfin enabled auto-merge (squash) September 20, 2022 19:01

fingolfin merged commit 43e0401 into oscar-system:master Sep 20, 2022

HechtiDerLachs mentioned this pull request Sep 23, 2022

Invariant theory pr #1564

Closed


		export SimpleGlueing

		@attributes mutable struct SimpleGlueing{LST<:AbsSpec,

Spec retying #1537

Spec retying #1537

Conversation

HechtiDerLachs commented Sep 13, 2022

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HechtiDerLachs commented Sep 15, 2022

afkafkafk13 commented Sep 15, 2022

HechtiDerLachs commented Sep 15, 2022

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

afkafkafk13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afkafkafk13 left a comment

Choose a reason for hiding this comment