test/ToricVarieties: fix+improve test for Stanley Reisner ideal

[benlorenz]

@HereAround
cc: @lkastner

Currently:
Linux CI (nightly):

Test Summary:                             | Pass  Total     Time
Additional test for Stanley-Reisner ideal |    1      1  2m37.9s

macOS CI (nightly):

Test Summary:                             | Pass  Total     Time
Additional test for Stanley-Reisner ideal |    1      1  5m13.4s

This PR should bring that down to probably a few seconds.

Also I don't really understand the point of this testcase, especially why is it running the the (fully deterministic) algorithm twice on the same input? The same wrong result would also be accepted by that test, maybe there is some other invariant of the ideal that could be precomputed and used for the test, maybe for some simpler example?

Oscar.jl/test/ToricVarieties/runtests.jl

Lines 202 to 209 in 9229804

	P = polarize(Polyhedron(Polymake.polytope.rand_sphere(5,60; seed=42)))
	big_variety = NormalToricVariety(P)
	big_variety2 = NormalToricVariety(P)
	set_coefficient_ring(big_variety2, GF(13))
	@testset "Additional test for Stanley-Reisner ideal" begin
	@test ngens(stanley_reisner_ideal(big_variety)) == ngens(stanley_reisner_ideal(big_variety2))
	end

Edit: With this PR:
Linux:

Test Summary:                             | Pass  Total  Time
Additional test for Stanley-Reisner ideal |    1      1  1.3s

macOS:

Test Summary:                             | Pass  Total  Time
Additional test for Stanley-Reisner ideal |    1      1  2.9s

[HereAround]

@benlorenz Thank you for spotting this. Unfortunately, your changes only conceal the real issue. Let me explain.

One of the important points of #939 was to speed up the computation of Stanley-Reisner ideals of toric varieties. The example that you are referring to was put forward by @micjoswig in #932. Back then, it took about 5 minutes and the runtime dropped to just a few seconds with the changes in #939.

My guess is therefore that one of the more recent changes to ToricVarieties has undone this performance optimization. Instead of changing the example, we should track this down and undo this change. (If any change to this example, then I would suggest to also measure the time it takes and issue and error if it takes longer than say 30 seconds, which is about 15 times more than I would expect once we revert to #939).

In fact, there is (at least a bit of) principle to the "madness" of this example. Namely, another crucial part of #939 was to allow for different coefficient rings. So besides computing the Stanley-Reisner ideal for a massive variety, I wanted to do it over a ring different from QQ or ZZ and opted for a Galois field.

The variety in question is constructed randomly. Even with the seed inserted (as you suggested at the time), I found that two runs gave different numbers of generators. (That was at least true when I added this example. I cannot test this right now.) So instead of just running the code, or checking something almost trivial (such as if there is at least one generator), I decided to compare the result with the default method in toric varieties. These two methods must provide the same answer, except for different coefficient rings. Therefore, this became in my humble opinion a much more interesting test.

Long story short: Please do not merge these changes. The real issue is (in my humble opinion) a rather different one and these changes just conceal it.

[benlorenz]

My guess is therefore that one of the more recent changes to ToricVarieties has undone this performance optimization. Instead of changing the example, we should track this down and undo this change. (If any change to this example, then I would suggest to also measure the time it takes and issue and error if it takes longer than say 30 seconds, which is about 15 times more than I would expect once we revert to #939).

This was caused by polymake choosing a different algorithm depending on what other properties are already present in the object, in this case a call to nvertices makes sure that it still uses the correct algorithm for the minimal non-faces (and this was removed during one of the refactorings to simplify the code).
I have added an @elapsed check to fail when the runtime is above 10 seconds, on my laptop it currently takes about 0.13 seconds.

The variety in question is constructed randomly. Even with the seed inserted (as you suggested at the time), I found that two runs gave different numbers of generators. (That was at least true when I added this example. I cannot test this right now.)

I cannot reproduce that and it really should not happen since all the algorithms are fully deterministic. Lets see if the CI does reproduce the same value that I put in the testcase now.

So instead of just running the code, or checking something almost trivial (such as if there is at least one generator), I decided to compare the result with the default method in toric varieties. These two methods must provide the same answer, except for different coefficient rings. Therefore, this became in my humble opinion a much more interesting test.

I don't really see where this would call different methods, for both varieties this will run the same code for the minimal non-faces from polymake and store these as generators for the ideal. Only the rings for these generators will be different but that should not affect the number of generators?

Long story short: Please do not merge these changes. The real issue is (in my humble opinion) a rather different one and these changes just conceal it.

It was explicitly marked as a draft and now contains a completely different changeset.

Edit:
Linux CI:

Test Summary:                             | Pass  Total  Time
Additional test for Stanley-Reisner ideal |    4      4  2.6s

macOS CI:

Test Summary:                             | Pass  Total  Time
Additional test for Stanley-Reisner ideal |    4      4  2.2s

Please note that this time includes multiple calls and some preparation so it is not comparable with the 0.12 seconds, and not the time limited by those 10 seconds.
And all test-runs seem to reproduce the same number of generators which I hardcoded now, 1648.

[HereAround]

Thank you for investigating @benlorenz. This sounds great!

Since the "random part" (probably I did a mistake back then) is now deterministic, I agree that only one Stanley-Reisner computation for this big variety is sufficient. I would suggest to keep the one with GF(13), so that the tests for ToricVarieties run at least one check with a field different from QQ.