More optimizations for integer functions #2061

fingolfin · 2018-01-03T15:08:09Z

This PR speeds up GcdInt, PValuation and RootInt; see the respective commit message for some timings.

UPDATE: also improved SmallestRootInt and IsPrimePowerInt

frankluebeck · 2018-01-04T15:47:39Z

This looks essentially good. But it breaks:

IsPrimeInt(100000000000000000000000000000000000000000000000151);
NextPrimeInt(10^50);

fingolfin · 2018-01-04T17:33:53Z

@frankluebeck could you please elaborate what is broken? I get the same output on this branch, master, and various older GAP versions, namely:

gap> IsPrimeInt(100000000000000000000000000000000000000000000000151);
true
gap> NextPrimeInt(10^50);
100000000000000000000000000000000000000000000000151

UPDATE: ah OK, if I load factint (which I do not do by default), I get an error. That's because factint abuses `RootInt to take roots of rationals.

gap> IsPrimeInt(100000000000000000000000000000000000000000000000151);
Error, Root: <n> must be an integer (not a rational) in
  r := ROOT_INT( n, k ); at /Users/mhorn/Projekte/GAP/gap.spielwiese/lib/integer.gi:1263 called from
RootInt( 2 ^ (LogInt( n, 10 ) - 50), 4 ) at /Users/mhorn/Projekte/GAP/gap.spielwiese/pkg/factint-1.5.4/lib/ecm.gi:323 called from
...
brk> n;
1/32
brk> k;
4

With the old code, we get this weird result (for unsupported inputs, of course, so there is nothing wrong with it per se)

gap> RootInt(1/32,4);
1

Ideally, factint should be fixed not to do that. Maybe @Stefan-Kohl can comment? Is this computation an accident, or is there really need for taking roots of rationals?

In the meantime, I could add a workaround for backwards compatibility, I'll just have to figure out what it ought to do.

(Finally, there is an undocumented operation Root, but so far its only installed method calls RootInt)

fingolfin · 2018-01-04T18:45:40Z

This patch for FactInt fixes the problem:

diff --git a/lib/ecm.gi b/lib/ecm.gi
index 80854ba..f41cd5b 100644
--- a/lib/ecm.gi
+++ b/lib/ecm.gi
@@ -319,8 +319,8 @@ function ( arg )
   if ArgCorrect then
     n      := GetArg(1,"DoesNotExist",fail);
     Curves := GetArg(2,"ECMCurves",
-                     n -> Maximum(4,3 * RootInt(2^(LogInt(n,10) - 24),8)
-                                      + RootInt(2^(LogInt(n,10) - 50),4)));
+                     n -> Maximum(4,3 * RootInt(Int(2^(LogInt(n,10) - 24)),8)
+                                      + RootInt(Int(2^(LogInt(n,10) - 50)),4)));
     if   IsInt(Curves)
     then NumberOfCurves := Curves; Curves := n -> NumberOfCurves; fi;
     if Curves(n) <= 0 then return [[],[n]]; fi;

There is a reason I don't load factint by default: primality testing seems to be much slower with it in cases relevant to me. E.g. the example given by Frank takes 22 milliseconds with me *without• FactInt; with FactInt, it's ~5 seconds.

fingolfin · 2018-01-04T19:19:50Z

I made a pull request for FactInt to correct the issue there, see gap-packages/FactInt#6

Moreover, I added a new commit to this PR which tweaks the primality prover, which (mostly) avoids the slowdown with FactInt loaded for me, and by coincidence (?) also the problem with RootInt.

Stefan-Kohl · 2018-01-04T20:43:34Z

I silently assumed that RootInt does something sensible if the first argument is a rational. If RootInt is changed such that this is no longer the case, the argument needs to be converted to an integer first, like in your fix. Thanks for pointing this out.

ChrisJefferson

Could we have a few tests for bigger ints, where we multiply some more things together?

fingolfin · 2018-01-08T21:54:32Z

@Stefan-Kohl RootInt never did anything sensible if the first argument was not an integer. Indeed, it even prints out an error message for non-integers -- but there was a bug in it, which caused it to silently accept any rational between 0 and 1 -- they were then treated as 1, i.e. RootInt(1/32,4) gave 1, which I wouldn't call "sensible" (0 would be)

@ChrisJefferson I am not sure I understand -- for what do you want additional tests?

BTW, this PR also has a test failure, which I don't understand right now: It seems some Random call now returns something different than before this PR. I am not yet sure why, though my suspicion is that factoring integers may invoke Random at some point, leading to the (harmless) test regression. I'll investigate next time I touch this PR (i.e. likely after @ChrisJefferson clarified and I then found some time to address his remarks)

fingolfin · 2018-01-08T21:59:17Z

lib/integer.gi

-    elif n < 0 and k mod 2 = 0  then Error("<n> must be positive");
-    elif n < 0 and k mod 2 = 1  then return -RootInt( -n, k );
-    elif n = 0                  then return 0;
-    elif n <= k                 then return 1;


This is the problem with the old code which caused it to accept some non-integer inputs: suppose n <= k holds (and none of the prior checks either handled the input, or indirectly caused an assertion, e.g. k mod 2 will error out if k is a rational with even denominator). Then it returns 1. This is only sensible if also 1 <= n holds, which is of course automatic if n is an integer (due to the preceding checks), but fails for rationals (such as e.g. n=1/32).

Since this "support" for rational inputs clearly was accidental (it's not documented; it's wrong for certain inputs; most rational inputs are not supported, e.g. RootInt(4+1/2, 2); always gave an error), I am strongly tempted to proceed with this PR as-is (targeting GAP 4.10, not 4.9), at least once a new FactInt version with the fix has been released.

Stefan-Kohl · 2018-01-08T22:03:55Z

In terms of FactInt, 1 was just as sensible as 0 -- and even 2 or 3 would hardly do more harm than causing a little slow-down on average ... . On the other hand, something like e.g. 100 would cause pretty poor performance in many cases.

ChrisJefferson · 2018-01-08T22:36:05Z

For tests, was just thinking if testing some more numbers which don't fit in a small integer, which have more factors, also big numbers with small mods, small numbers with big mods. Nothing too serious.

fingolfin · 2018-01-09T09:20:18Z

@ChrisJefferson I have now extended the tests, which revealed a bug in the new ROOT_INT(n,k) if k was a non-immediate negative number, or if k is not immediate and n=0. So, good suggestion ;-). Found no other issues.
Also added a commit to fix the regression in the test for Random.

It remains to be decided whether I should add a workaround to RootInt to accept formally invalid calls made by FactInt, which used to work; or if we instead wait for a new FactInt release which fixes this. Maybe also @alex-konovalov has a thought on that?

olexandr-konovalov · 2018-01-10T15:12:57Z

@fingolfin new FactInt release within a week, maybe - would that be OK then?

hulpke

Looks good to me.

Before: gap> SumX([0 .. 100000], [2,3,5,7,251,256], RootInt);; time; 784 gap> SumX([0 .. 100000], [2,3,5,7,251,256], {n,k}->RootInt(2^100+n,k));; time; 1890 After: gap> SumX([0 .. 100000], [2,3,5,7,251,256], RootInt);; time; 293 gap> SumX([0 .. 100000], [2,3,5,7,251,256], {n,k}->RootInt(2^100+n,k));; time; 775

Namely, by passing it the 'cheap' option (which implies the FactIntPartial option), as the surrounding code would suggest. This makes calling e.g. IsPrimeInt(100000000000000000000000000000000000000000000000151); with FactInt loaded almost as fast as without it loaded.

fingolfin · 2018-01-15T15:12:07Z

Rebased this, now that PR #2086 has been merged. Only three commits remain in here. Still waiting for new FactInt release, on which @alex-konovalov is currently working

hulpke · 2018-01-15T15:42:44Z

@fingolfin #2087 is so far only in my work branch. Shall I put it into master?
Or is the cleaner way to add it to this PR?

fingolfin · 2018-01-15T16:16:12Z

@hulpke sorry, that was a typo, I meant #2086 and not #2087

olexandr-konovalov · 2018-01-29T20:52:26Z

Added to release notes for GAP 4.10 at https://github.com/gap-system/gap/wiki/GAP-4.10-release-notes (one common entry for this PR and #2086).

fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: kernel topic: performance bugs or enhancements related to performance (improvements or regressions) labels Jan 3, 2018

fingolfin added this to the GAP 4.10.0 milestone Jan 3, 2018

fingolfin force-pushed the mh/more-int-opts branch 2 times, most recently from 1bd66d6 to b5dd9cb Compare January 4, 2018 08:59

ChrisJefferson reviewed Jan 8, 2018

View reviewed changes

fingolfin commented Jan 8, 2018

View reviewed changes

fingolfin force-pushed the mh/more-int-opts branch 2 times, most recently from 0ef031a to 1978125 Compare January 9, 2018 09:17

fingolfin force-pushed the mh/more-int-opts branch from 1978125 to 370a093 Compare January 9, 2018 14:36

stevelinton mentioned this pull request Jan 10, 2018

Dealing with Old Versions of Packages that are Broken by Releases; add "Conflicts" field to PackageInfo.g? #2077

Open

hulpke approved these changes Jan 10, 2018

View reviewed changes

This was referenced Jan 13, 2018

Speed up GcdInt, PValuation, SmallestRootInt, IsPrimePowerInt #2086

Merged

FactorsInt in library #2087

Closed

fingolfin added 3 commits January 15, 2018 16:10

Make some Random() tests resilient to seed changes

9e04c56

fingolfin force-pushed the mh/more-int-opts branch from 370a093 to facf179 Compare January 15, 2018 15:10

markuspf merged commit 17cc656 into gap-system:master Jan 17, 2018

fingolfin deleted the mh/more-int-opts branch January 18, 2018 08:12

olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jan 29, 2018

olexandr-konovalov mentioned this pull request Feb 17, 2018

Update CHANGES gap-packages/FactInt#11

Closed

More optimizations for integer functions #2061

More optimizations for integer functions #2061

Uh oh!

Conversation

fingolfin commented Jan 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frankluebeck commented Jan 4, 2018

Uh oh!

fingolfin commented Jan 4, 2018

Uh oh!

fingolfin commented Jan 4, 2018

Uh oh!

fingolfin commented Jan 4, 2018

Uh oh!

Stefan-Kohl commented Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChrisJefferson left a comment

Choose a reason for hiding this comment

Uh oh!

fingolfin commented Jan 8, 2018

Uh oh!

fingolfin Jan 8, 2018

Choose a reason for hiding this comment

Uh oh!

Stefan-Kohl commented Jan 8, 2018

Uh oh!

ChrisJefferson commented Jan 8, 2018

Uh oh!

fingolfin commented Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olexandr-konovalov commented Jan 10, 2018

Uh oh!

hulpke left a comment

Choose a reason for hiding this comment

Uh oh!

fingolfin commented Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hulpke commented Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fingolfin commented Jan 15, 2018

Uh oh!

olexandr-konovalov commented Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fingolfin commented Jan 3, 2018 •

edited

Loading

Stefan-Kohl commented Jan 4, 2018 •

edited

Loading

fingolfin commented Jan 9, 2018 •

edited

Loading

fingolfin commented Jan 15, 2018 •

edited

Loading

hulpke commented Jan 15, 2018 •

edited

Loading

olexandr-konovalov commented Jan 29, 2018 •

edited

Loading