New implementation of map_from_func with changed argument order

[mjrodgers]

Closes #4741

There is a lot of confusion from the methods map_from_func and MapFromFunc and their similar names; one first step to resolving this is to try to convert all existing uses of map_from_func in Oscar to use MapFromFunc instead.

This PR also

• excludes the AbstractAlgebra version of map_from_func from being imported from Hecke
• introduces a new OSCAR version of map_from_func as an alias to MapFromFunc

As a result, we now have that the version of map_from_func available in Oscar

• takes arguments in the new order (D, C, fun [, inv]), as opposed to the prior (fun, D, C)
• returns an object of type MapFromFunc, as opposed to the prior FunctionalMap.

[mjrodgers]

For some reason these changes are breaking AbstractVarietyMap, the pushforward field is not being set correctly...

julia> P2 = abstract_projective_space(2)
AbstractVariety of dim 2

julia> P5 = abstract_projective_space(5, symbol = "H")
AbstractVariety of dim 5

julia> h = gens(P2)[1]
h

julia> i = map(P2, P5, [2*h])
AbstractVarietyMap from AbstractVariety of dim 2 to AbstractVariety of dim 5

julia> i.pushforward
ERROR: UndefRefError: access to undefined reference
Stacktrace:
 [1] getproperty(x::AbstractVarietyMap{AbstractVariety, AbstractVariety}, f::Symbol)
   @ Base ./Base.jl:37
 [2] top-level scope
   @ REPL[13]:1

this causes problems later... this seemed to be set correctly when map_from_func was used (on main):

julia> P2 = abstract_projective_space(2)
AbstractVariety of dim 2

julia> P5 = abstract_projective_space(5, symbol = "H")
AbstractVariety of dim 5

julia> h = gens(P2)[1]
h

julia> i = map(P2, P5, [2*h])
AbstractVarietyMap from AbstractVariety of dim 2 to AbstractVariety of dim 5

julia> i.pushforward
Map defined by a Julia function
  from quotient of multivariate polynomial ring by ideal (h^3)
  to quotient of multivariate polynomial ring by ideal (H^6)

[mjrodgers]

Ahhh, okay this is because the AbstractVarietyMap has the field pushforward::FunctionalMap, but we now have a MapFromFunc instead. I think this is fixed now.

[mjrodgers]

As near as I can tell, this new error:

julia> P2 = abstract_projective_space(2)
AbstractVariety of dim 2

julia> P5 = abstract_projective_space(5)
AbstractVariety of dim 5

julia> i = map(P2, P5, [2P2.O1])
AbstractVarietyMap from AbstractVariety of dim 2 to AbstractVariety of dim 5

julia> Bl, E, j = blow_up(i)
ERROR: ArgumentError: Image not in the codomain
Stacktrace:
 [1] macro expansion
   @ ~/.julia/packages/AbstractAlgebra/5aJmh/src/Assertions.jl:602 [inlined]
 [2] image(f::MapFromFunc{MPolyQuoRing{…}, MPolyQuoRing{…}}, x::MPolyQuoRingElem{MPolyDecRingElem{…}})
   @ Hecke ~/.julia/packages/Hecke/RwzTY/src/Map/MapType.jl:196
 [3] (::MapFromFunc{MPolyQuoRing{…}, MPolyQuoRing{…}})(x::MPolyQuoRingElem{MPolyDecRingElem{…}})
   @ Hecke ~/.julia/packages/Hecke/RwzTY/src/Hecke.jl:612
 [4] pushforward(f::AbstractVarietyMap{AbstractVariety, AbstractVariety}, F::AbstractBundle{AbstractVariety})
   @ Oscar.IntersectionTheory ~/.julia/dev/Oscar/experimental/IntersectionTheory/src/Main.jl:583
 [5] blow_up(i::AbstractVarietyMap{AbstractVariety, AbstractVariety}; symbol::String)
   @ Oscar.IntersectionTheory ~/.julia/dev/Oscar/experimental/IntersectionTheory/src/blowup.jl:209
 [6] blow_up(i::AbstractVarietyMap{AbstractVariety, AbstractVariety})
   @ Oscar.IntersectionTheory ~/.julia/dev/Oscar/experimental/IntersectionTheory/src/blowup.jl:82
 [7] top-level scope
   @ REPL[61]:1
Some type information was truncated. Use `show(err)` to see complete types.

arises because FunctionalMap did not check that the image of an element lies in the codomain, and MapFromFunc does check...

I think this line 198 from experimental/IntersectionTheory/src/blowup.jl
states the wrong domain, it should be ABl instead of AX. Changing it to see if that works.

[codecov]

Codecov Report

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

Project coverage is 84.88%. Comparing base (c4a6aae) to head (c7837a3).
Report is 9 commits behind head on master.

Files with missing lines	Patch %	Lines
...raicGeometry/Surfaces/EllipticSurface/Morphisms.jl	33.33%	4 Missing ⚠️
experimental/IntersectionTheory/src/blowup.jl	60.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4988      +/-   ##
==========================================
- Coverage   84.88%   84.88%   -0.01%     
==========================================
  Files         697      698       +1     
  Lines       94190    94191       +1     
==========================================
  Hits        79956    79956              
- Misses      14234    14235       +1     

Files with missing lines	Coverage Δ
...ental/IntersectionTheory/src/IntersectionTheory.jl	100.00% <ø> (ø)
experimental/IntersectionTheory/src/Main.jl	93.31% <100.00%> (ø)
experimental/IntersectionTheory/src/Types.jl	90.24% <100.00%> (ø)
src/Groups/abelian_aut.jl	98.55% <100.00%> (-0.01%)	⬇️
src/Misc/MapFromFunc.jl	100.00% <100.00%> (ø)
src/imports.jl	0.00% <ø> (ø)
experimental/IntersectionTheory/src/blowup.jl	66.93% <60.00%> (ø)
...raicGeometry/Surfaces/EllipticSurface/Morphisms.jl	62.81% <33.33%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[lgoettgens]

Looks good to me, but it would be great if @wdecker or @thofma could have a look at the IntersectionTheory changes

[mjrodgers]

I guess to finish this up, I need to deprecate map_from_func, i'll try to take care of that today.

[lgoettgens]

I guess to finish this up, I need to deprecate map_from_func, i'll try to take care of that today.

To do that you could put map_from_func into exclude_hecke in

Oscar.jl/src/imports.jl

Line 184 in 546c0fc

let exclude_hecke = [

to prevent it from getting imported into Oscar. And then add @deprecate map_from_func(func, dom, codom) MapFromFunc(dom, codom, func) to deprecations.jl to create a new function map_from_func in Oscar that delegates to MapFromFunc but prints a deprecation warning

[lgoettgens]

Or if we want to keep the current behaviour, still add it to the exclude_hecke and add the following to deprecations.jl:

function map_from_func(func, dom, codom)
  Base.depwarn("something")
  return AbstractAlgebra.map_from_func(func, dom, codom)
end

[fingolfin]

Nobody in triage had objections to this PR

[fingolfin]

One more option: we could do

function map_from_func(func, dom, codom)
  return MapFromFunc(dom, codom, func)
end

[mjrodgers]

One more option: we could do

function map_from_func(func, dom, codom)
  return MapFromFunc(dom, codom, func)
end

I like this idea best, of map_from_func actually calling MapFromFunc. For the variant where we give an inverse, should this be called as map_from_func(func, inv, dom, codom)?

[thofma]

There was some discussion before and the consensus was that we want the convention

map_from_func(dom, codom, fun [, inv])

[mjrodgers]

Is there an existing file where it makes sense to put this code? Or should I make a new file in src/Misc/?

[wdecker]

I think this line 198 from experimental/IntersectionTheory/src/blowup.jl
states the wrong domain, it should be ABl instead of AX. Changing it to see if that works.

@mjrodgers Thx for catching this! I will add a test as soon as this PR is merged.

[wdecker]

@mjrodgers The intersection theory changes look good to me. To double check the bug correction you made, try

P2 = abstract_projective_space(2);
P5 = abstract_projective_space(5);
i = map(P2, P5, [2P2.O1]);
Bl, E, j = blow_up(i);
AE = chow_ring(E);
CE = j.pushforward(one(AE))

The result you should get is e (it works on my machine):

[mjrodgers]

Hi Wolfram, Yes, we get `e` on both the old and new versions. However in the current `main` branch we have ```julia-repl julia> parent(j.pushforward(one(AE))) Quotient of multivariate polynomial ring in 2 variables over QQ graded by e -> [1] h -> [1] by ideal (2*e^3 - 9*e^2*h + 15*e*h^2 - 8*h^3, e*h^3) julia> codomain(j.pushforward) Quotient of multivariate polynomial ring in 1 variable over QQ graded by h -> [1] by ideal (h^6) ``` because `map_from_func` returns a `FunctionalMap` which does not check that the image actually lies in the codomain. In the new version with the correction I made to the comain, these now match.

[mjrodgers]

I've added in this (in a new file in Misc), along with a docstring borrowed from Hecke. Hopefully this is all we need to wrap this one up, but if there are any more tweaks needed let me know and I'm happy to implement them.

Actually scratch that, I should probably redo all of the MapFromFunc calls to use this new map_from_func. That should be quick though.

[mjrodgers]

Or maybe I should leave the MapFromFunc constructors for now, there are almost 300 usages in Oscar (that could be fixed with a find/replace, but not sure if this is necessary)

[lgoettgens]

Originally, this PR just tried to replace all internal uses of map_from_func by MapFromFunc, which doesn't need release notes IMO.
However, now the main change is a changed order of arguments in map_from_func, which should IMO be mentioned in the release notes.

[fingolfin]

@lgoettgens OK like this? I wonder if we should add the renaming label it is not really a renaming from the user's perspective, but I don't really see any other category that fits better....?