Oscar.jl icon indicating copy to clipboard operation
Oscar.jl copied to clipboard

Some bugfixes and improvements

Open HechtiDerLachs opened this issue 9 months ago • 10 comments

This is work in progress for all the small fixes necessary to make some old scripts of @simonbrandhorst and myself run again.

HechtiDerLachs avatar May 02 '24 11:05 HechtiDerLachs

General question: Do you have any idea, what increased the cost of these older scripts? I find this a bit scary! To me this sounds like some optimization (at some other and maybe completely unrelated) point having very negative (and up to now undetected) side effects.

The improvements make sense in any case. I have used such tweaks many times for large settings in Singular, where primary decomposition as a black box did not go through. But you need to be aware that you are deliberately circumventing the heuristics of primdec.lib and this should only be necessary in nasty examples. Is your offending example really sufficiently nasty?

afkafkafk13 avatar May 02 '24 15:05 afkafkafk13

Thanks for your comments.

No, in my opinion the offending example is not nasty. I sent it to @hannes14 via email, but I can put it here again:

julia> minimal_primes(list[2])
R = Multivariate polynomial ring in 4 variables over QQ
gens(R) = QQMPolyRingElem[θ_2, θ_1, (s0//s1), (w//x)]
g = -θ_1^2*(s0//s1) + θ_1^2 + (s0//s1)*(w//x)^4
g = θ_2^8 + 1
g = -θ_2^6 + θ_2^4 - θ_2^2 + θ_1^2

Be aware that s0//s1 etc. are not fractions, but only symbols for variable names. So this is really a polynomial computation and the three gs are the generators of the ideal in question (printed in a for-loop with g as local variable).

To me this did not look scary at all. But minimal_primes gives up on this with GTZ.

HechtiDerLachs avatar May 02 '24 15:05 HechtiDerLachs

Does anyone have an explanation for the failing tests? I suppose the same things are tested in other threads, aren't they? Why do they fail on ubuntu 1.10 long?

@lgoettgens ?

HechtiDerLachs avatar May 02 '24 18:05 HechtiDerLachs

Does anyone have an explanation for the failing tests? I suppose the same things are tested in other threads, aren't they? Why do they fail on ubuntu 1.10 long?

@lgoettgens ?

ubuntu-1.10-long spent 2.5 hours in /home/runner/work/Oscar.jl/Oscar.jl/test/AlgebraicGeometry/Schemes/elliptic_surface.jl and timed out

withoutexperimental spent about 2 hours in /home/runner/work/Oscar.jl/Oscar.jl/test/AlgebraicGeometry/Schemes/elliptic_surface.jl and timed out

ubuntu-1.10-book failed due to some change in output in decker-schmitt-invariant-theory: https://github.com/oscar-system/Oscar.jl/actions/runs/8927137202/job/24519841523?pr=3672#step:8:3632

lgoettgens avatar May 02 '24 19:05 lgoettgens

Thanks for your comments.

No, in my opinion the offending example is not nasty. I sent it to @hannes14 via email, but I can put it here again:

julia> minimal_primes(list[2])
R = Multivariate polynomial ring in 4 variables over QQ
gens(R) = QQMPolyRingElem[θ_2, θ_1, (s0//s1), (w//x)]
g = -θ_1^2*(s0//s1) + θ_1^2 + (s0//s1)*(w//x)^4
g = θ_2^8 + 1
g = -θ_2^6 + θ_2^4 - θ_2^2 + θ_1^2

Be aware that s0//s1 etc. are not fractions, but only symbols for variable names. So this is really a polynomial computation and the three gs are the generators of the ideal in question (printed in a for-loop with g as local variable).

To me this did not look scary at all. But minimal_primes gives up on this with GTZ.

I entered the computation in Singular and found the following observations: ring r=0,(a,b,c,d),dp; ideal I=b^2*c+b^2+c*d^4, a^8+1, -a^6+a^4-a^2+b^2; internally uses a ring (0,c),(a,b,d),(lp(3),C) and stalls

ring r=0,(d,c,b,a),dp; ideal I=b^2*c+b^2+c*d^4, a^8+1, -a^6+a^4-a^2+b^2; internally uses a ring (0,d),(c,b,a),(lp(3),C) and returns the result in a fraction of a second.

So the difference is in the internal choice of the "maximal independent set" in this primary decomposition algorithm and of course in the difficulty of the lp-groebner basis to be computed internally. This choice is not done by an elaborate heuristic; hence the situation is brittle, whenever the different choices have significantly different runtimes. So probalby some change further up in the hierarchy changed the sequence of the variables, which in turn triggers a different choice of a maximal independent set. Actually, the most solid (but currently rather unrealistic) variant would be to run the different choices in parallel and let the first one win. (Maybe you want to at least make sure that the variables from the tower of algebraic field extensions are added in a suitable order to avoid unnecessary spoly among the minimal polynomials w.r.t. lex ordering.)

afkafkafk13 avatar May 02 '24 21:05 afkafkafk13

I changed the default algorithm for the place where primary decomposition is called in the elliptic surfaces. Maybe that already helps to have the tests run more reliably (#3676).

HechtiDerLachs avatar May 03 '24 08:05 HechtiDerLachs

The change to minimal_primes changes the ordering in one of the book tests. How do we handle this again? Otherwise I would like to have this in, so that the CI stops running into #3676 (I hope).

joschmitt avatar May 07 '24 07:05 joschmitt

The change to minimal_primes changes the ordering in one of the book tests. How do we handle this again? Otherwise I would like to have this in, so that the CI stops running into #3676 (I hope).

You can change the output line in the jlcon file (test/book/specialized/decker-schmitt-invariant-theory/H3.jlcon).

benlorenz avatar May 07 '24 07:05 benlorenz

I adjusted the output of the failing book test.

joschmitt avatar May 07 '24 12:05 joschmitt

Codecov Report

Attention: Patch coverage is 69.11765% with 21 lines in your changes are missing coverage. Please review.

Project coverage is 81.31%. Comparing base (789bd83) to head (70da52f). Report is 25 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3672      +/-   ##
==========================================
- Coverage   82.89%   81.31%   -1.59%     
==========================================
  Files         577      577              
  Lines       78308    78568     +260     
==========================================
- Hits        64916    63886    -1030     
- Misses      13392    14682    +1290     
Files Coverage Δ
experimental/Schemes/critical_locus.jl 0.00% <ø> (ø)
src/Rings/mpoly-ideals.jl 93.02% <100.00%> (+0.36%) :arrow_up:
src/Rings/mpolyquo-localizations.jl 71.11% <100.00%> (-0.44%) :arrow_down:
src/Rings/MPolyMap/MPolyQuo.jl 88.88% <33.33%> (-2.78%) :arrow_down:
experimental/Schemes/elliptic_surface.jl 4.44% <0.00%> (-70.64%) :arrow_down:
experimental/Schemes/IdealSheaves.jl 72.21% <36.36%> (-5.85%) :arrow_down:
...erimental/Schemes/MorphismFromRationalFunctions.jl 39.93% <20.00%> (-7.53%) :arrow_down:

... and 67 files with indirect coverage changes

codecov[bot] avatar May 07 '24 13:05 codecov[bot]

I would suggest to merge this and relegate further improvements to a new PR.

simonbrandhorst avatar May 08 '24 07:05 simonbrandhorst

Good to go from my side.

HechtiDerLachs avatar May 08 '24 12:05 HechtiDerLachs