Further tuning on flattenings of rings

[HechtiDerLachs]

Some more steps towards addressing #4112 . Still WIP but seemingly a speedup of factor 10 already. Timings will follow. I think, this is what I can do for the moment.

[HechtiDerLachs]

Timings: Again, I am using this polynomial as a benchmark.

before:

julia> f = load("/path/to/poly.txt");

julia> g = copy(f);

julia> S = parent(f);

julia> Oscar.flatten(S);

julia> @time Oscar.flatten(S)(g);
  0.227188 seconds (1.05 M allocations: 46.635 MiB, 53.65% gc time)

julia> g = copy(f);

julia> @time Oscar.flatten(S)(g);
  0.099856 seconds (1.00 M allocations: 46.005 MiB)

julia> g = copy(f);

julia> @time Oscar.flatten(S)(g);
  0.112565 seconds (1.40 M allocations: 57.970 MiB)

julia> g = copy(f);

julia> @time Oscar.flatten(S)(g);
  0.244818 seconds (1.39 M allocations: 56.852 MiB, 46.52% gc time)

after:

julia> f = load("/path/to/poly.txt");

julia> g = copy(f);

julia> S = parent(f);

julia> Oscar.flatten(S);

julia> @time Oscar.flatten(S)(g);
  0.054546 seconds (578.67 k allocations: 27.150 MiB)

julia> g = copy(f);

julia> @time Oscar.flatten(S)(g);
  0.061309 seconds (578.67 k allocations: 27.150 MiB)

julia> g = copy(f);

julia> @time Oscar.flatten(S)(g);
  0.039068 seconds (428.25 k allocations: 20.028 MiB)

So roughly a factor 2 in improvement for the special cases covered compared to current master.

[thofma]

Are you caching the image of elements by default? Or why is the copy necessary in the benchmark?

[HechtiDerLachs]

It used to be caching by default. I turned it off here, because I realized that this might lead to bugs with in-place operations. The copy could therefore be skipped in these tests, really.

[codecov]

Codecov Report

Attention: Patch coverage is 94.73684% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.40%. Comparing base (23b0b9d) to head (7fa42e9).
Report is 9 commits behind head on master.

Files with missing lines	Patch %	Lines
src/Rings/MPolyMap/flattenings.jl	77.77%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4400   +/-   ##
=======================================
  Coverage   84.40%   84.40%           
=======================================
  Files         656      657    +1     
  Lines       87216    87299   +83     
=======================================
+ Hits        73612    73688   +76     
- Misses      13604    13611    +7     

Files with missing lines	Coverage Δ
src/Rings/MPolyMap/flattening_specializations.jl	100.00% <100.00%> (ø)
src/Rings/MPolyMap/flattenings.jl	92.25% <77.77%> (-0.71%)	⬇️

... and 7 files with indirect coverage changes

[HechtiDerLachs]

On a first glance it seems to me that none of the failing tests is really my fault. Correct me if I'm wrong!

Otherwise this should be good to go.

[HechtiDerLachs]

Picking up on the timings by @fingolfin on #4112 done for the tests of the EllipticSurfaces:

current master: 310.391684 seconds (2.06 G allocations: 72.914 GiB, 12.26% gc time, 0.12% compilation time)
with this PR: 310.526301 seconds (2.08 G allocations: 73.502 GiB, 12.21% gc time, 0.24% compilation time)
(edit/update) with this PR: 303.933921 seconds (2.05 G allocations: 72.303 GiB, 11.67% gc time)

So either the expensive tasks lay elsewhere, or the improvements here just don't pay off as much as anticipated. Both are not unlikely, the first because flattening of rings only plays a role in the computation of gluings which is not the main objective of the algorithms tested there. For the second: We do not have more fine-tuned methods for polynomial- and quotient rings over localizations (yet). These come up naturally in the gluings as we usually glue over principal open subsets. I left a comment in the source code to indicate this as a potential thread for future work. But as this is not a super urgent matter right now, I would prefer to spend my resources differently for the moment.

[jankoboehm]

Picking up on the timings by @fingolfin on #4112 done for the tests of the EllipticSurfaces:

current master: 310.391684 seconds (2.06 G allocations: 72.914 GiB, 12.26% gc time, 0.12% compilation time) with this PR: 310.526301 seconds (2.08 G allocations: 73.502 GiB, 12.21% gc time, 0.24% compilation time)

So either the expensive tasks lay elsewhere, or the improvements here just don't pay off as much as anticipated. Both are not unlikely, the first because flattening of rings only plays a role in the computation of gluings which is not the main objective of the algorithms tested there. For the second: We do not have more fine-tuned methods for polynomial- and quotient rings over localizations (yet). These come up naturally in the gluings as we usually glue over principal open subsets. I left a comment in the source code to indicate this as a potential thread for future work. But as this is not a super urgent matter right now, I would prefer to spend my resources differently for the moment.

Agree there, crucial algorithmic problems are elsewhere.

[lgoettgens]

The nightly, book failure is due to #4402, so can be disregarded here.

[simonbrandhorst]

Looks good to me. Thanks for tuning the code.