Resolution structure (and curve case)

[afkafkafk13]

previously: Only for collaboration on this task!

Now: structure of desingularization framework plus curve case with naive choice of center as the testing ground for the structure

[HechtiDerLachs]

Some tests still need to be adjusted. But other than that, I think this is rather fine as a working base. The adjustments can be made in the end, or at any time where we would like to have a good picture of the test suite.

[afkafkafk13]

Thank you for the help and the preparation for monday.

[afkafkafk13]

Currently untested: Framework for handling embedded setting.

[codecov]

Codecov Report

Merging #3480 (4b2c800) into master (39a24f2) will decrease coverage by 0.01%.
Report is 1 commits behind head on master.
The diff coverage is 69.60%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3480      +/-   ##
==========================================
- Coverage   82.10%   82.10%   -0.01%     
==========================================
  Files         566      567       +1     
  Lines       76123    76615     +492     
==========================================
+ Hits        62504    62901     +397     
- Misses      13619    13714      +95     

Files	Coverage Δ
experimental/Schemes/CoveredProjectiveSchemes.jl	88.40% <100.00%> (+7.04%)	⬆️
experimental/Schemes/ToricBlowups/types.jl	65.21% <ø> (ø)
...raicGeometry/Schemes/Covering/Morphisms/Methods.jl	97.74% <100.00%> (ø)
...metry/Schemes/PrincipalOpenInclusion/Attributes.jl	100.00% <100.00%> (ø)
src/Modules/ExteriorPowers/Generic.jl	91.05% <100.00%> (+0.07%)	⬆️
experimental/Schemes/IdealSheaves.jl	81.88% <88.88%> (+2.78%)	⬆️
experimental/Schemes/BlowupMorphism.jl	90.09% <71.42%> (-0.46%)	⬇️
experimental/Schemes/SimplifiedAffineScheme.jl	82.03% <56.25%> (-1.65%)	⬇️
experimental/Schemes/Resolution_structure.jl	68.54% <68.54%> (ø)

... and 4 files with indirect coverage changes

[fingolfin]

@afkafkafk13 asked me to have a look at the technical side of this PR (I can't comment on the math). Overall it seems fine to me. I left a few suggestions, but they are mostly optional (maybe handle the one with the many calls to maps which each allocate, but even that can be done later if you prefer)

[fingolfin]

If I understand your code right, then maps(phi) each time returns a fresh copy of the vector maps. So better to call it only once. (This might affect other places, too, I didn't check)

Suggested change

	len=length(maps(phi))
	result=underlying_morphism(maps(phi)[1])
	for i in 2:len
	result = compose(underlying_morphism(maps(phi)[i]), result)
	end
	m = maps(phi)
	len = length(m)
	result = underlying_morphism(m[1])
	for i in 2:len
	result = compose(underlying_morphism(m[i]), result)
	end

Alrernatively you could also use mapreduce here:

Suggested change

	len=length(maps(phi))
	result=underlying_morphism(maps(phi)[1])
	for i in 2:len
	result = compose(underlying_morphism(maps(phi)[i]), result)
	end
	result = mapreduce(underlying_morphism, compose, maps(phi))

That should be equivalent... maybe the loop is clearer, though. Just wanted to mention it as an option

[HechtiDerLachs]

I suggest not to return the fully composed morphism here, but an instance of CompositeCoveredSchemeMorphism. Everything is already implemented for that one and like this all the special functionality for the latter will automatically be harvested.

[HechtiDerLachs]

@fingolfin : Thanks for having a look. I agree with your suggestions.

Concerning the maps: I propose to introduce a method map(f, i) here as a direct getter in accordance with what we already have for ComplexOfMorphisms. I know that this is debated as map also stands for something more general. But it is what we have in Oscar in many other cases at the time and it would only be coherent if we introduced it also here. If at some later point we decide to clean this up, then its usage will also be cleaned up here.

[fingolfin]

If Oscar.map is a distinct (and then necessarily non-exported) function from Base.map, I guess we could live with it... But overall this I find this very problematic, and it runs completely counter to the definition of Base.map, and even has the same arity as it has.

IMHO at the very least this needs to be run by @fieker and @thofma.

[simonbrandhorst]

Suggested change

	AbsDesingMor{
	AbsDesingMor{

Please consider renaming to AbsDesingularizationMor.

[simonbrandhorst]

Why copy? Typically our getters do not copy stuff. The user should assume that anything returned is unsafe to mutate?
... but it is a common pitfall

[HechtiDerLachs]

@afkafkafk13 and I have talked about this. I suggested to return a copy. Otherwise, the chances are high that unsafe code leads to a mess with the internals of the structure. And this tends to be almost impossible to debug. The way to solve this, is to provide the getter with the index and encourage the user to use that instead of creating copies all over the place.

And I could live with morphisms and morphism below.

[afkafkafk13]

taking your other suggestions for morphisms instead of maps into account, I would go for

morphisms(phi::AbsDesingMor) = phi.maps
morphisms_copy(phi::AbsDesingMor) = copy(phi.maps)
morphism(phi::AbsDesingMor, i::Int) = phi.maps[i]

Do you agree? Note that morphisms_copy is necessary at certain places, because the desingularization has to modify it by appending. I will carefully go through the existing code and change to morphisms_copy precisely, where it is necessary.

[HechtiDerLachs]

No morphisms_copy, please! morphisms should create a copy each time and not give access to the internals. Such things have proved to be devastating in many instances. IMHO saving memory allocations has to be achieved by other means and this is also possible, e.g. by encouraging the use of morphism(phi, i).

[HechtiDerLachs]

Typically our getters do not copy stuff.

I do not think that this is true. Even gens for polynomial rings usually creates a copy. And it does so for good reasons.

[afkafkafk13]

ok then just morphisms(phi) and morphism(phi,i) giving back copies. I will make the appropriate changes.

[simonbrandhorst]

best rename .maps to .morphisms as well for consistency.

[fingolfin]

Regarding copy in morphisms (and similar elsewhere in gens etc.): for OSCAR 2.0 we could consider using a type like the one defined in https://github.com/bkamins/ReadOnlyArrays.jl which implements the AbstractArray / AbstractVector interface by wrapping an existing array/vector and passing all operations through -- except anything that writes to the vector. The whole thing is basically "free" in terms of overhead.

But the downside is that the result is not a Vector anymore -- code which accepts AbstractVector will keep working, but surely a lot of code will need (trivial) adjustments -- hence this can't go into Oscar 1.x

In this regard, it would be much better if Array just had a bit flag that turned any attempt to write to it into an error

[afkafkafk13]

best rename .maps to .morphisms as well for consistency.

That was part of the content of my last statement.

[simonbrandhorst]

How about replacing maps by morphisms? Then there is no clash.

[HechtiDerLachs]

@afkafkafk13 : I took the liberty to implement the AbsBlowdownMorphism interface we once worked out. Simply because I was curious whether this was doable and we were recently working on this topic with @HereAround for the toric blowups.

In particular, I resolved the suggestion about the underlying morphism which I pointed out above.