Skip to content
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

Disjoint gluings #3861

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Disjoint gluings #3861

wants to merge 7 commits into from

Conversation

simonbrandhorst
Copy link
Collaborator

@simonbrandhorst simonbrandhorst commented Jun 14, 2024

Introduces disjoint gluings.
In particular when we ask a covering for a gluing which is a disjoint union, before it would give a
key error in a dictionary. Not it returns an empty gluing.

@simonbrandhorst
Copy link
Collaborator Author

simonbrandhorst commented Jun 14, 2024

This change breaks the gluing graph. Unfortunately that is not documented. Hence I can only guess.

Copy link

codecov bot commented Jun 14, 2024

Codecov Report

Attention: Patch coverage is 86.36364% with 6 lines in your changes missing coverage. Please review.

Project coverage is 62.24%. Comparing base (5e624ff) to head (aea5294).
Report is 25 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (5e624ff) and HEAD (aea5294). Click for more details.

HEAD has 3 uploads more than BASE | Flag | BASE (5e624ff) | HEAD (aea5294) | |------|------|------| ||7|10|
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #3861       +/-   ##
===========================================
- Coverage   81.83%   62.24%   -19.60%     
===========================================
  Files         581      578        -3     
  Lines       80002    82889     +2887     
===========================================
- Hits        65468    51591    -13877     
- Misses      14534    31298    +16764     
Files Coverage Δ
experimental/Schemes/src/LazyGluing.jl 89.47% <ø> (-8.61%) ⬇️
...cGeometry/Schemes/Covering/Objects/Constructors.jl 100.00% <100.00%> (ø)
...ebraicGeometry/Schemes/Covering/Objects/Methods.jl 79.63% <100.00%> (-1.47%) ⬇️
...c/AlgebraicGeometry/Schemes/Gluing/Constructors.jl 64.94% <100.00%> (-5.84%) ⬇️
src/AlgebraicGeometry/Schemes/Gluing/Types.jl 100.00% <ø> (ø)
...chemes/PrincipalOpenSubset/Objects/Constructors.jl 100.00% <100.00%> (ø)
...try/Schemes/CoveredSchemes/Objects/Constructors.jl 94.73% <0.00%> (-5.27%) ⬇️
...metry/Schemes/PrincipalOpenSubset/Objects/Types.jl 60.00% <66.66%> (+6.66%) ⬆️
src/AlgebraicGeometry/Schemes/Gluing/Attributes.jl 71.87% <40.00%> (-20.72%) ⬇️

... and 397 files with indirect coverage changes

@afkafkafk13
Copy link
Collaborator

This change breaks the gluing graph. Unfortunately that is not documented. Hence I can only guess.

As far as I see it, the new disjoint_union provides the correct covering of the disjoint union of the two schemes covered by the coverings, whereas the other one provided the disjoint union of two coverings. Both are legitimate and useful functionality. How do we do the naming right?
In the end, we might for instance say disjoint_union for the old functionality (i.e. disjoint union of coverings) and cover_disjoint_union for the new one.

Concerning the other issue: the gluing graph breaks, because is_dense is only defined for PrincipalOpenSubset and AffineSchemeOpenSubscheme, but not for the empty scheme. This needs to be caught in update_gluing_graph in src/AlgebraicGeometry/Schemes/Covering/Objects/Methods.jl line63 saying something like
typeof(C[X,Y]) != DisjointGluing || continue

@simonbrandhorst
Copy link
Collaborator Author

This change introduces bugs in many more places .... they are just not covered by the tests.

The reason is that so far the gluings expect the gluing domains to be of type
PrincipalOpenSubset or AffineSchemeOpenSubscheme

So what are our options for the gluing domain types?
1 . PrincipalOpenSubset currently cannot be empty, since we did not allow localisation at zero ... perhaps we should?
2. AffineSchemeOpenSubscheme can be empty and is allowed ... but is rather underdeveloped
3. AffineScheme R/1 ... some parts of the interface do not make sense anymore such as is_dense ... the problem is that it does not know in what it is an open subset in. I don't think it is sustainable in the long run.

We have to test that
fiber products, restrictions, base_change .... all work with our new type of gluing
For 1. I think it is going to be okay
For 2. This could result in problems if we start mixing PrincipalOpens with AffineSchemeOpenSubscheme
For 3. no clue what to expect

@simonbrandhorst
Copy link
Collaborator Author

Now the 1. suggestion is implemented. It turned out the principal opens are flexible enough.
I expect that there will be some fallout/corner cases, since the existing code worked without an interface.
@HechtiDerLachs will know where to look.
But I think meanwhile this is good to go/be tested

@simonbrandhorst simonbrandhorst marked this pull request as ready for review June 14, 2024 13:47
@HechtiDerLachs
Copy link
Collaborator

Can we briefly discuss why introducing "disjoint gluings" became necessary? Maybe through another channel, though.

Up to now the rationale is the following: If two open subschemes are not glued, then the gluing is not there. You can check whether or not the corresponding key exists in the dictionary. If that seems to be too complicated, we could introduce a function to ask whether two affine schemes are glued.

Localizing at zero was forbidden for several reasons (which I unfortunately do not recall in detail right now). I do not think that allowing this solves more problems than it creates.

That the gluing domains are either PrincipalOpenSubsets or AffineSchemeOpenSubschemes is a bit painful, but a natural consequence of the fact that for a long time people were convinced that gluings should happen along the latter. Even though I still have not seen a single practical use case. But now we have to deal with it. You can estimate from the type of the gluing what the domains will be like. Maybe we have to clean up the access to such information a bit.

The gluing_graph is still a bit experimental, as I recall. It's use would have been more in the context of gluings along AffineSchemeOpenSubscheme where we would compute new gluings by extrapolating from others. That approach is not really pursued anymore and we should probably look into its usefulness again.

@afkafkafk13
Copy link
Collaborator

Not for starting a discussion, just for information:
Handling pullback ideal sheaves and the like in a lazy setting (which is nowadays used automatically) requires checking a haskey for a lazy dict. One idea was to make the disjoint unions explicit. There are alternatives: If we would have something like has_key_lazy which passes down via the underlying_gluings until it gets its hand on a dict and then does a haskey, this would already help. The laziness of the gluing and the disjoint gluings via non-existence of keys in the dict do not get along well as they are right now.

@simonbrandhorst
Copy link
Collaborator Author

simonbrandhorst commented Jun 16, 2024

Introducing a 0 x n matrix solves a lot more problems than it creates. Similarly you can ask x^3+1 for its coefficient at degree 4 and get a sensible answer - zero and not an error.
In this vein, I think that allowing a disjoint gluing is better than throwing a key not found error. The coordinate ring is implemented as R/1 and thus not as localisation at 0. The complement equation(s) is 0, which could create some problems down the road.
Well and as Anne mentioned, when things are computed at request, perhaps the dictionary is not completely filled.

@simonbrandhorst
Copy link
Collaborator Author

@afkafkafk13 could you please provide an example for your problem where you try to iterate over the non-empty gluings but hit an empty gluing?

@afkafkafk13
Copy link
Collaborator

@afkafkafk13 could you please provide an example for your problem where you try to iterate over the non-empty gluings but hit an empty gluing?

After the last PRs by @HechtiDerLachs , the offending examples now work as expected. He fixed the problem in a different way than this PR, but it looks fixed for the moment.

@HechtiDerLachs
Copy link
Collaborator

I was wondering whether what I did would fix your problem. It was a bug in the code, not necessarily about missing "empty gluings". But we can pick up on that discussion at some later point.

Since the original problem is solved for the moment, I close this for now.

@afkafkafk13
Copy link
Collaborator

I agree.

@simonbrandhorst simonbrandhorst marked this pull request as draft June 24, 2024 21:07
@simonbrandhorst
Copy link
Collaborator Author

If it is closed, we will never look at it again. I still think that having empty gluings can be beneficial.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants