Some more fixes

[HechtiDerLachs]

Today's harvest from working on the elliptic surfaces.

[thofma]

I think a lot of changes from #4133 and #4126 are undone here. I guess this was not on purpose.

[HechtiDerLachs]

Yes, I'm already working on it. It's a bit annoying, because I had to work on the compute-server today, but there I can't put my login-credentials for GH. Thus, I manually copied the files via scp and this is what you get...

unwanted changes removed

[lgoettgens]

Suggested change

	pre_result = filter!(!is_one, vcat([minimal_primes(j; algorithm, factor_generators=false) for j in J]...))
	pre_result = filter!(!is_one, reduce(vcat, [minimal_primes(j; algorithm, factor_generators=false) for j in J]))

save some allocations

[HechtiDerLachs]

I advocate for this solution now. It is a matter of fact that ideals can not be hashed in general and even for polynomial rings this is probably expensive if not impossible (we need to choose an ordering which might be a dead end for a specific ideal). Yet, the == function is vital to how we use ideals and we need to be able to use them in vectors/lists/dictionaries.

[simonbrandhorst]

Is it safe to hash the type? After all two mathematically equal objects may have different types.
(Thinking e.g. of IdealSheaf, RadicalIdealSheaf, PrimeIdealSheaf etc.)

[lgoettgens]

Is it safe to hash the type? After all two mathematically equal objects may have different types. (Thinking e.g. of IdealSheaf, RadicalIdealSheaf, PrimeIdealSheaf etc.)

It is only safe if you don't implement == between this type and other types. In your example, you shouldn't include the type in hashes.

[HechtiDerLachs]

Ok, good to know. I was only following the suggestion by @benlorenz . But you're probably right.

[HechtiDerLachs]

I am sorry about the messy state that this is in. But it turned out to be a really hard nut to crack here.

Some things to be discussed:

1. I introduced a "simultaneous blowup strategy" for the elliptic surfaces. Something which is vital to what we try to achieve these days. This should actually be consensus.

2. Because of 1) I needed to restructure the implementation of restrict(::AbsCoveredSchemeMorhpism, ::CoveredClosedEmbedding, ::CoveredClosedEmbedding). This lead to a lot of issues down the road. All code which uses refinements of coverings for morphisms of covered schemes is still rather brittle. Unfortunately, this is a bigger task to clean this up. A lot has already been done, but many things still only work in very particular constellations. One thing which did not work anymore was the isomorphism_on_open_subset which I had once added in order to be able to identify function fields along birational maps like blowups. This feature turned out to not be that useful and we actually don't use it these days. So I took the liberty to disable it. Let's see whether the tests pass. Ping @simonbrandhorst

3. Something went wrong with some composition of maps. I suddenly got CompositeMaps where earlier the composition had been carried out. I still could not pin down the original cause of this, but I made some fixes. In the course of these events, I found it necessary to move the type declaration for MPolyAnyMap to a file of its own. Otherwise, debugging would not have been possible. I would appreciate it, if it could stay this way. Ping @thofma .

Once this reaches a state where tests actually pass, I will remove the code which for the time being has just been commented out. But if you have any objections about where parts of this are going, please let me know already.

[afkafkafk13]

Can you clarify what you mean by 'simultaneous blowup strategy'?

[HechtiDerLachs]

Can you clarify what you mean by 'simultaneous blowup strategy'?

In the elliptic surfaces we exclusively encounter A-D-E singularities. Up to now we blew them up one by one. But this produces a lot of charts and gluings. In the particular example we need for our paper we only have 8 ordinary A1 singularities. They are resolved simultaneously by one single blowup. This is achieved with with a keyword argument to the constructor for elliptic surfaces which then selects this strategy: Just blow up the ideal sheaf of the reduced singular locus, until you're done.

[afkafkafk13]

Can you clarify what you mean by 'simultaneous blowup strategy'?

In the elliptic surfaces we exclusively encounter A-D-E singularities. Up to now we blew them up one by one. But this produces a lot of charts and gluings. In the particular example we need for our paper we only have 8 ordinary A1 singularities. They are resolved simultaneously by one single blowup. This is achieved with with a keyword argument to the constructor for elliptic surfaces which then selects this strategy: Just blow up the ideal sheaf of the reduced singular locus, until you're done.

oh. I see. This makes a lot of sense.

[simonbrandhorst]

Before the refinements were registered. Now this does not seem to be the case anymore.
Is this intentional? And if yes what is the rationale here?

[HechtiDerLachs]

Originally we were planning to keep track of all the refinements and the morphisms between them inside the covered scheme. But this is not very garbage-collector-friendly, so we eventually refrained from that and switched to the implicit tree structure for refinements. The call to _register! was a deprecated artefact.

[simonbrandhorst]

I am fine with discontinuing isomorphism_on_open_subset it is in experimental and not exported.

[simonbrandhorst]

The changes in the doctests in unrelated code such as algebraic statistics seem odd.
Can you explain the change in behavior? Are the results still correct?

[HechtiDerLachs]

Can you explain the change in behavior?

See #4171 and my comment on slack.

[simonbrandhorst]

O.K. when you checkout the files I reapprove.

[codecov]

Codecov Report

Attention: Patch coverage is 60.59480% with 106 lines in your changes missing coverage. Please review.

Project coverage is 84.56%. Comparing base (4875c11) to head (a03191c).

Files with missing lines	Patch %	Lines
experimental/Schemes/src/elliptic_surface.jl	13.59%	89 Missing ⚠️
.../AlgebraicGeometry/Schemes/Divisors/base_change.jl	0.00%	9 Missing ⚠️
experimental/Schemes/src/BlowupMorphism.jl	76.47%	4 Missing ⚠️
experimental/Schemes/src/BlowupMorphismTypes.jl	92.59%	2 Missing ⚠️
src/Rings/MPolyMap/MPolyAnyMap.jl	93.75%	1 Missing ⚠️
src/Rings/mpoly-ideals.jl	97.14%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4142      +/-   ##
==========================================
- Coverage   84.69%   84.56%   -0.14%     
==========================================
  Files         628      631       +3     
  Lines       84578    84673      +95     
==========================================
- Hits        71636    71606      -30     
- Misses      12942    13067     +125     

Files with missing lines	Coverage Δ
...perComplexes/src/Morphisms/simplified_complexes.jl	94.71% <100.00%> (+0.09%)	⬆️
experimental/Schemes/src/Types.jl	78.37% <ø> (-6.00%)	⬇️
...etry/Schemes/AffineSchemes/Morphisms/Attributes.jl	85.45% <ø> (+5.79%)	⬆️
...eometry/Schemes/AffineSchemes/Morphisms/Methods.jl	91.73% <100.00%> (-1.74%)	⬇️
...ometry/Schemes/AffineSchemes/Objects/Attributes.jl	90.47% <ø> (ø)
...cGeometry/Schemes/AffineSchemes/Objects/Methods.jl	92.27% <100.00%> (+0.21%)	⬆️
...try/Schemes/CoveredSchemes/Morphisms/Attributes.jl	85.71% <ø> (+19.04%)	⬆️
...raicGeometry/Schemes/Covering/Morphisms/Methods.jl	98.64% <ø> (ø)
...ebraicGeometry/Schemes/Covering/Objects/Methods.jl	80.97% <100.00%> (-0.22%)	⬇️
...icGeometry/Schemes/FunctionField/FunctionFields.jl	76.89% <ø> (-1.14%)	⬇️
... and 11 more

... and 6 files with indirect coverage changes