Speed up polynomial mappings #4124

HechtiDerLachs · 2024-09-19T15:46:14Z

@fingolfin mentioned in his course this week that in the RingFlattenings there was lots of potential improvements available via the use of MPolyBuildCtx. I tried a little bit, but could not achieve any better performance. I'm wondering why, though, so I put it up here for discussion.

simonbrandhorst · 2024-09-20T06:58:28Z

src/Rings/MPolyMap/MPolyRing.jl

+    r = ngens(S)
+    ctx = MPolyBuildCtx(S)
+    for (c, e) in zip(AbstractAlgebra.coefficients(u), AbstractAlgebra.exponent_vectors(u))
+      ee = [0 for _ in 1:r]


Suggested change

ee = [0 for _ in 1:r]

ee = Vector{Int}(undef, r)

should be an epsilon faster

One should be able to use the "same" vector ee for the whole loop as it gets copied anyway. I try to optimize this further.

An undefined vector will not work. We need the zeroes.

src/Rings/MPolyMap/MPolyRing.jl

src/Rings/MPolyMap/MPolyAnyMap.jl

HechtiDerLachs · 2024-09-25T07:31:14Z

Unfortunately this still does not seem to make a difference. I was running the tests for elliptic surfaces with and without the changes made here (since @fingolfin pointed to that as a potential example). The result is

Without this PR:

500.418518 seconds (2.93 G allocations: 112.836 GiB, 12.64% gc time, 0.28% compilation time)

With this PR:

508.444454 seconds (2.90 G allocations: 111.645 GiB, 12.23% gc time, 0.16% compilation time)

The different timings might also be due to randomization of parts of the tests on the lattice theoretic side (@simonbrandhorst : Is this correct?), but the allocations do not go down significantly.

One thing I noticed is that finish does a lot of sorting (which has already been pointed out to me by @lgoettgens ). This creates some overhead. But in general, another problem I witnessed with the RingFlattenings is that we have massive is_zero checks whenever we do arithmetic with SRows. These can not be avoided. But when we do this with a quotient ring, then we always need to translate the representative to the Singular-side. If this quotient ring is put to work via flattenings, then additionally we first have to translate to the flattened ring. This kills performance for sure.

In my opinion it would therefore be desirable to have a singular_representative in a MPolyQuoRingElem and to do the arithmetic with these by default whenever they are defined. Only memorizing whether an element is reduced does not help in these cases.

Orthogonal to that, the question remains: Why does this PR not help speeding things up for mappings?

HechtiDerLachs · 2024-09-25T10:24:49Z

So: In the end this is useful. Just not for all the flattenings used in the elliptic surfaces (yet). I created the following test polynomial.

Run the following code:

g = load("poly.txt")
S = parent(g)
flat = Oscar.flatten(S)
f = copy(g);
@time flat(f);

With this PR:


julia> f = copy(g);

julia> @time flat(f);
  0.335777 seconds (686.51 k allocations: 49.636 MiB)

julia> f = copy(g);

julia> @time flat(f);
  0.334014 seconds (690.14 k allocations: 48.630 MiB)

julia> f = copy(g);

julia> @time flat(f);
  0.328450 seconds (705.82 k allocations: 49.011 MiB)

julia> f = copy(g);

julia> @time flat(f);
  0.330575 seconds (711.82 k allocations: 49.188 MiB)

without this PR:

julia> f = copy(g);

julia> @time flat(f);
  4.988692 seconds (975.34 k allocations: 654.598 MiB, 27.35% gc time)

julia> f = copy(g);

julia> @time flat(f);
  5.196591 seconds (750.61 k allocations: 652.844 MiB, 27.13% gc time)

julia> f = copy(g);

julia> @time flat(f);
  5.379392 seconds (673.50 k allocations: 652.256 MiB, 26.06% gc time)

julia> f = copy(g);

julia> @time flat(f);
  5.556177 seconds (659.54 k allocations: 652.183 MiB, 25.55% gc time)

thofma · 2024-09-25T19:14:28Z

Looks good so far. I think this is going in the right direction. (It reminds of the MPolyHom_vars we once had and some ideas of Dan).

The logic for the evaluation was already hard to follow before (blame on me), and it seems it won't be getting better. But it is easer to talk in persons about this.

HechtiDerLachs · 2024-09-30T11:52:16Z

I added a keyword argument to the internal constructor for the maps to be able to avoid the internal checks whether you have a mapping of variables to variables. These are not yet forwarded to the user-facing constructors, but should this ever become a performance issue, this possibility exists.

HechtiDerLachs · 2024-09-30T13:35:52Z

Looks like we're slowly getting somewhere here. I ran the elliptic_surfaces.jl test and got the following:
Before:

560.589288 seconds (3.01 G allocations: 119.270 GiB, 10.71% gc time, 21.51% compilation time)

after:

484.957044 seconds (2.48 G allocations: 92.375 GiB, 11.66% gc time, 0.02% compilation time)

ping: @fingolfin

src/AlgebraicGeometry/Schemes/AffineSchemes/Morphisms/Attributes.jl

This reverts commit 0c65e56.

This reverts commit f005ce1.

HechtiDerLachs · 2024-09-30T18:12:18Z

@HereAround : Might it be the case that your test in the FTheoryTools has some randomness? See this commit with the local changes from my machine.

Anyways: If the tests pass, then from my side, this is good to go.

codecov · 2024-09-30T18:13:37Z

Codecov Report

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

Project coverage is 84.69%. Comparing base (2fe164e) to head (1892dab).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/Rings/MPolyMap/MPolyRing.jl	98.14%	1 Missing ⚠️
src/Rings/mpolyquo-localizations.jl	97.77%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4124      +/-   ##
==========================================
+ Coverage   84.68%   84.69%   +0.01%     
==========================================
  Files         628      628              
  Lines       84471    84578     +107     
==========================================
+ Hits        71533    71636     +103     
- Misses      12938    12942       +4

Files with missing lines	Coverage Δ
...Schemes/AffineSchemeOpenSubscheme/Rings/Methods.jl	`86.86% <100.00%> (+0.09%)`	⬆️
...etry/Schemes/AffineSchemes/Morphisms/Attributes.jl	`79.66% <ø> (ø)`
...ry/Schemes/AffineSchemes/Morphisms/Constructors.jl	`100.00% <ø> (ø)`
...ometry/Schemes/AffineSchemes/Objects/Attributes.jl	`90.47% <ø> (ø)`
src/Rings/MPolyMap/MPolyAnyMap.jl	`93.10% <100.00%> (+0.24%)`	⬆️
src/Rings/MPolyMap/MPolyRing.jl	`94.11% <98.14%> (+4.53%)`	⬆️
src/Rings/mpolyquo-localizations.jl	`73.52% <97.77%> (+0.60%)`	⬆️

... and 1 file with indirect coverage changes

lgoettgens · 2024-09-30T18:15:51Z

@HereAround : Might it be the case that your test in the FTheoryTools has some randomness? See this commit with the local changes from my machine.

Anyways: If the tests pass, then from my side, this is good to go.

Unfortunately these kind of doctests (printing a large vector) are dependent on the windows size of the terminal you run the doctest_fix from

src/Rings/MPolyMap/MPolyRing.jl

thofma · 2024-09-30T18:24:37Z

I added a keyword argument to the internal constructor for the maps to be able to avoid the internal checks whether you have a mapping of variables to variables. These are not yet forwarded to the user-facing constructors, but should this ever become a performance issue, this possibility exists.

Why does this exist if it is not used here?

HechtiDerLachs · 2024-09-30T18:28:00Z

Why does this exist if it is not used here?

I thought, it would be nice to give future developers a hint that they can safely disable this feature by using the keyword argument. If you think, it's causing more confusion than insight, then I can remove it. Also, it seems that we can reunite both internal constructors into one constructor again, now that I have allowed more rings in the codomain.

thofma · 2024-09-30T19:03:52Z

I thought, it would be nice to give future developers a hint that they can safely disable this feature by using the keyword argument. If you think, it's causing more confusion than insight, then I can remove it. Also, it seems that we can reunite both internal constructors into one constructor again, now that I have allowed more rings in the codomain.

I had hoped that a no one would ever create a map by calling MPolyAnyMap directly :) I am happy to keep it. I just wanted to ask why this unused (and not tested) feature exists.

HereAround · 2024-09-30T19:59:34Z

@HereAround : Might it be the case that your test in the FTheoryTools has some randomness? See this commit with the local changes from my machine.

Anyways: If the tests pass, then from my side, this is good to go.

The computation of generic_section of line bundles picks random coefficients. This is the element of randomness in the FTheoryTools test. Everything else is (or so I hope at least) deterministic.

thofma · 2024-10-01T06:45:21Z

src/Rings/mpolyquo-localizations.jl

+# The following methods are only safe, because they are called 
+# exclusively in a setup where variables map to variables 
+# and no denominators are introduced. 
+_coefficients(x::MPolyRingElem) = coefficients(x)


Not sure, but maybe you want the following here?

Suggested change

_coefficients(x::MPolyRingElem) = coefficients(x)

_coefficients(x::MPolyRingElem) = AbstractAlgebra.coefficients(x)

And also below with exponent_vectors.

I saw the AA-qualifier elsewhere, too. But I thought, it was redundant. Isn't it?

No. It is the difference between painfully slow and fast when iterating over coefficients/exponents. There used to be an explanation in the documentation, but it was removed for reasons I don't know.

HechtiDerLachs · 2024-10-01T11:09:11Z

Again: Ready for merge from my side.

thofma · 2024-10-01T12:58:36Z

Thanks, it looks good.

src/Rings/MPolyMap/MPolyAnyMap.jl

joschmitt marked this pull request as draft September 19, 2024 15:55

simonbrandhorst reviewed Sep 20, 2024

View reviewed changes

HechtiDerLachs commented Sep 20, 2024

View reviewed changes

src/Rings/MPolyMap/MPolyRing.jl Outdated Show resolved Hide resolved

fingolfin changed the title ~~Speedup polynomial mappings~~ Speed up polynomial mappings Sep 20, 2024

fingolfin reviewed Sep 20, 2024

View reviewed changes

src/Rings/MPolyMap/MPolyAnyMap.jl Outdated Show resolved Hide resolved

src/Rings/MPolyMap/MPolyAnyMap.jl Outdated Show resolved Hide resolved

src/Rings/MPolyMap/MPolyAnyMap.jl Outdated Show resolved Hide resolved

HechtiDerLachs marked this pull request as ready for review September 30, 2024 13:36

HechtiDerLachs added 14 commits September 30, 2024 18:28

Attempt to speed up mapping of polynomials.

6fd471a

Some more steps towards making stuff faster.

70279f9

Try to make general evaluation faster.

4304881

Clean up general evaluation.

a96cf36

Fix tests.

b6a3c40

Widen type signature so that more rings benefit from variable-map.

d1f90e3

Fix call.

f98f198

Implement extension for localizations and quotients.

570e769

Switch default for _allunique.

2d83dd6

Fix tests.

9a59613

Remove superfluous if-clause and add some comments.

4e5b8e7

Fix doctests in Algebraic statistics.

f005ce1

Fix doctests in FTheoryTools.

0c65e56

Fix doctests in AffineSchemes.

8944bb2

HechtiDerLachs force-pushed the speedup_polynomial_mappings branch from e11b048 to 8944bb2 Compare September 30, 2024 16:28

lgoettgens reviewed Sep 30, 2024

View reviewed changes

src/AlgebraicGeometry/Schemes/AffineSchemes/Morphisms/Attributes.jl Show resolved Hide resolved

HechtiDerLachs added 2 commits September 30, 2024 19:00

Revert "Fix doctests in FTheoryTools."

90dc095

This reverts commit 0c65e56.

Revert "Fix doctests in Algebraic statistics."

5d61dda

This reverts commit f005ce1.

thofma reviewed Sep 30, 2024

View reviewed changes

src/Rings/MPolyMap/MPolyRing.jl Outdated Show resolved Hide resolved

HechtiDerLachs added 4 commits September 30, 2024 22:30

Remove duplicated code.

7ad7f39

Revert to a single internal constructor.

4d17d35

Fix internal method.

f7289e8

Remove some more code duplication.

9092a8a

thofma reviewed Oct 1, 2024

View reviewed changes

Switch to the AA methods.

733b8f1

thofma approved these changes Oct 1, 2024

View reviewed changes

thofma reviewed Oct 1, 2024

View reviewed changes

src/Rings/MPolyMap/MPolyAnyMap.jl Outdated Show resolved Hide resolved

simonbrandhorst reviewed Oct 1, 2024

View reviewed changes

src/Rings/MPolyMap/MPolyAnyMap.jl Outdated Show resolved Hide resolved

Update src/Rings/MPolyMap/MPolyAnyMap.jl

1892dab

simonbrandhorst enabled auto-merge (squash) October 1, 2024 13:22

simonbrandhorst merged commit 4875c11 into oscar-system:master Oct 1, 2024
28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speed up polynomial mappings #4124

Speed up polynomial mappings #4124

HechtiDerLachs commented Sep 19, 2024

simonbrandhorst Sep 20, 2024

joschmitt Sep 20, 2024

HechtiDerLachs Sep 20, 2024

HechtiDerLachs commented Sep 25, 2024

HechtiDerLachs commented Sep 25, 2024 •

edited

Loading

thofma commented Sep 25, 2024

HechtiDerLachs commented Sep 30, 2024

HechtiDerLachs commented Sep 30, 2024

HechtiDerLachs commented Sep 30, 2024 •

edited

Loading

codecov bot commented Sep 30, 2024 •

edited

Loading

lgoettgens commented Sep 30, 2024

thofma commented Sep 30, 2024

HechtiDerLachs commented Sep 30, 2024

thofma commented Sep 30, 2024

HereAround commented Sep 30, 2024 •

edited

Loading

thofma Oct 1, 2024

HechtiDerLachs Oct 1, 2024

thofma Oct 1, 2024

thofma Oct 1, 2024

HechtiDerLachs commented Oct 1, 2024

thofma commented Oct 1, 2024

	_coefficients(x::MPolyRingElem) = coefficients(x)
	_coefficients(x::MPolyRingElem) = AbstractAlgebra.coefficients(x)

Speed up polynomial mappings #4124

Speed up polynomial mappings #4124

Conversation

HechtiDerLachs commented Sep 19, 2024

simonbrandhorst Sep 20, 2024

Choose a reason for hiding this comment

joschmitt Sep 20, 2024

Choose a reason for hiding this comment

HechtiDerLachs Sep 20, 2024

Choose a reason for hiding this comment

HechtiDerLachs commented Sep 25, 2024

HechtiDerLachs commented Sep 25, 2024 • edited Loading

thofma commented Sep 25, 2024

HechtiDerLachs commented Sep 30, 2024

HechtiDerLachs commented Sep 30, 2024

HechtiDerLachs commented Sep 30, 2024 • edited Loading

codecov bot commented Sep 30, 2024 • edited Loading

Codecov Report

lgoettgens commented Sep 30, 2024

thofma commented Sep 30, 2024

HechtiDerLachs commented Sep 30, 2024

thofma commented Sep 30, 2024

HereAround commented Sep 30, 2024 • edited Loading

thofma Oct 1, 2024

Choose a reason for hiding this comment

HechtiDerLachs Oct 1, 2024

Choose a reason for hiding this comment

thofma Oct 1, 2024

Choose a reason for hiding this comment

thofma Oct 1, 2024

Choose a reason for hiding this comment

HechtiDerLachs commented Oct 1, 2024

thofma commented Oct 1, 2024

HechtiDerLachs commented Sep 25, 2024 •

edited

Loading

HechtiDerLachs commented Sep 30, 2024 •

edited

Loading

codecov bot commented Sep 30, 2024 •

edited

Loading

HereAround commented Sep 30, 2024 •

edited

Loading