Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix build and tests with pari 2.17 #38749

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

antonio-rojas
Copy link
Contributor

NEXT_PRIME_VIADIFF is removed in 2.17, port pari_prime_range to pari_PRIMES instead

Needs sagemath/cypari2#165 applied to cypari

@antonio-rojas
Copy link
Contributor Author

Needs work because this is not compatible with 2.15. So either it needs to be merged with the pari upgrade or be made to work with older pari too (no idea how).

Running tests now.

@antonio-rojas antonio-rojas changed the title Fix build with pari 2.17 [WIP] Fix build with pari 2.17 Oct 3, 2024
@antonio-rojas
Copy link
Contributor Author

Many test failures.

@antonio-rojas antonio-rojas changed the title [WIP] Fix build with pari 2.17 [WIP] Fix build and tests with pari 2.17 Oct 3, 2024
@jhpalmieri
Copy link
Member

If the changes here eventually allow us to use a system Pari 2.17, then we should undo the change in #38772.

@antonio-rojas antonio-rojas changed the title [WIP] Fix build and tests with pari 2.17 Fix build and tests with pari 2.17 Oct 6, 2024
@antonio-rojas
Copy link
Contributor Author

antonio-rojas commented Oct 6, 2024

With this MR and the cypari fixes sagemath/cypari2#165 and sagemath/cypari2#166 all tests are passing with pari 2.17.

The changes are not compatible with 2.15 though, making them compatible requires more work. Also, some pari opeations (such as the number field prime ideals above a given prime) give random output with 2.17, which makes it harder to test. To solve both issues (and make tests more future proof), we should gradually move away from testing the exact output to just testing that the output is correct.

@tornaria
Copy link
Contributor

tornaria commented Nov 4, 2024

There seems to be a major issue with pari 2.17: the computation of the units of a bnf is not deterministic:

? setrand(1) ; bnfinit(x^2-65).fu
%1 = [Mod(x - 8, x^2 - 65)]
? setrand(3) ; bnfinit(x^2-65).fu
%2 = [Mod(-x + 8, x^2 - 65)]

And also (related?) generators of ideals:

$ sage -c 'pari.setrand(1) ; K.<a> = QuadraticField(-6) ; L.<b> = K.extension(polygen(K)^4 + a) ; print(L.ideal(b).relative_norm())'
Fractional ideal (-a)
$ sage -c 'pari.setrand(3) ; K.<a> = QuadraticField(-6) ; L.<b> = K.extension(polygen(K)^4 + a) ; print(L.ideal(b).relative_norm())'
Fractional ideal (a)

and factorization are non-deterministic as well:

$ sage -c 'pari.setrand(1) ; R.<omega> = EisensteinIntegers() ; print(factor(3+omega))'
(omega) * (-3*omega - 2)
$ sage -c 'pari.setrand(2) ; R.<omega> = EisensteinIntegers() ; print(factor(3+omega))'
(omega + 1) * (-2*omega + 1)
$ sage -c 'pari.setrand(4) ; R.<omega> = EisensteinIntegers() ; print(factor(3+omega))'
omega + 3
$ sage -c 'pari.setrand(7) ; R.<omega> = EisensteinIntegers() ; print(factor(3+omega))'
(-omega) * (3*omega + 2)
$ sage -c 'pari.setrand(24) ; R.<omega> = EisensteinIntegers() ; print(factor(3+omega))'
(-1) * (-omega - 3)
$ sage -c 'pari.setrand(44) ; R.<omega> = EisensteinIntegers() ; print(factor(3+omega))'
(-omega - 1) * (2*omega - 1)

This is making tests a bit unstable, because running in --long or not reaches the examples with different pari random state...

vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 5, 2024
    
We need to remove this function from cypari2, because pari 2.17 has
changed precision from words to bits which would cause confusion.

Besides, the current usage is incorrect, since `log_pari.precision()`
will give the precision in *decimal digits* not in words.

See: sagemath#38749

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
    
URL: sagemath#38908
Reported by: Gonzalo Tornaría
Reviewer(s): Vincent Delecroix
vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 6, 2024
    
We need to remove this function from cypari2, because pari 2.17 has
changed precision from words to bits which would cause confusion.

Besides, the current usage is incorrect, since `log_pari.precision()`
will give the precision in *decimal digits* not in words.

See: sagemath#38749

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
    
URL: sagemath#38908
Reported by: Gonzalo Tornaría
Reviewer(s): Vincent Delecroix
@antonio-rojas
Copy link
Contributor Author

There seems to be a major issue with pari 2.17: the computation of the units of a bnf is not deterministic:

Yes, see #38749 (comment). I have only tested with --long and still find failing tests from time to time due to this. The reasonable solution is to make all tests independent of the exact output, but that is a lot of work.

@antonio-rojas
Copy link
Contributor Author

I am also seeing one test failure with a specific random seed:

sage -t --long --warn-long 30.0 --random-seed=164564093241797715927265696104150551458 /usr/lib/python3.12/site-packages/sage/rings/integer.pyx
**********************************************************************
File "/usr/lib/python3.12/site-packages/sage/rings/integer.pyx", line 3074, in sage.rings.integer.Integer.divisors
Failed example:
    for i in range(20):           # long time                             # needs sage.libs.pari
        try:
            alarm(RDF.random_element(1e-3, 0.5))
            _ = n.divisors()
            cancel_alarm()  # we never get here
        except AlarmInterrupt:
            pass
Exception raised:
    Traceback (most recent call last):
      File "<doctest sage.rings.integer.Integer.divisors[20]>", line 4, in <module>
        _ = n.divisors()
            ^^^^^^^^^^^^
      File "sage/rings/integer.pyx", line 3160, in sage.rings.integer.Integer.divisors (build/cythonized/sage/rings/integer.c:31916)
        for p, e in f:
      File "/usr/lib/python3.12/site-packages/sage/structure/factorization.py", line 327, in __getitem__
        def __getitem__(self, i):
        
      File "src/cysignals/signals.pyx", line 341, in cysignals.signals.python_check_interrupt
    cysignals.signals.AlarmInterrupt

    During handling of the above exception, another exception occurred:

    Traceback (most recent call last):
      File "/usr/lib/python3.12/site-packages/sage/doctest/forker.py", line 715, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/lib/python3.12/site-packages/sage/doctest/forker.py", line 1136, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.rings.integer.Integer.divisors[20]>", line 6, in <module>
        except AlarmInterrupt:
               ^^^^^^^^^^^^^^
      File "/usr/lib/python3.12/site-packages/sage/doctest/util.py", line 497, in __getitem__
        def __getitem__(self, name):
        
      File "src/cysignals/signals.pyx", line 341, in cysignals.signals.python_check_interrupt
    cysignals.signals.AlarmInterrupt
**********************************************************************

This is not reproducible with pari 2.15, but I don't know if it's because of some pari change or because the pari upgrade influences the choice of random timeouts. The obvious fix (adding from cysignals import AlarmInterrupt as in line 7119) doesn't help.

@tornaria
Copy link
Contributor

tornaria commented Nov 6, 2024

@antonio-rojas I can reproduce it deterministically with pari 2.15 and 2.17 with this change:

--- a/src/sage/rings/integer.pyx	2024-08-23 10:46:19.000000000 -0300
+++ b/src/sage/rings/integer.pyx	2024-11-06 11:23:36.666702122 -0300
@@ -3075,9 +3075,9 @@
         so a memory leak will not go unnoticed)::
 
             sage: n = prod(primes_first_n(25))                                          # needs sage.libs.pari
-            sage: for i in range(20):           # long time                             # needs sage.libs.pari
+            sage: for t in [0.5, 0.003]:
             ....:     try:
-            ....:         alarm(RDF.random_element(1e-3, 0.5))
+            ....:         alarm(t)
             ....:         _ = n.divisors()
             ....:         cancel_alarm()  # we never get here
             ....:     except AlarmInterrupt:

I couldn't reproduce this outside doctesting this file.

EDIT: the point is that with this change, the failure happens every time, regardless of seed, and regardless of --long. The randomness is just hitting a sequence of timeouts where this triggers (i.e. a very small timeout after a largish timeout as I did up there).

@tornaria
Copy link
Contributor

tornaria commented Nov 6, 2024

There seems to be a major issue with pari 2.17: the computation of the units of a bnf is not deterministic:

Yes, see #38749 (comment). I have only tested with --long and still find failing tests from time to time due to this. The reasonable solution is to make all tests independent of the exact output, but that is a lot of work.

Sure, but there is still something weird with this. Some of the E.isogeny_class() computations take a very long time (randomly) and I'm wondering if this is the cause.

I found other issues with reduced_basis() which I tracked back to the new LLL being quite unstable (numerically) and in that case I solved by reverting to the old LLL. Now I am wondering if all this unstability in units, factoring, generators, may be due to the same LLL unstability.

In principle, this new FLATTER version of LLL is better (faster) but...

@tornaria
Copy link
Contributor

tornaria commented Nov 6, 2024

BTW: I'm preparing a patch for 32-bit. Besides all the unstability, pari 2.17 changed the default real precision to be 128 bits for both 64-bit and 32-bit arches (previously it was 128 bits = 38 digits for 64-bit arch and 96 bit = 28 digits for 32-bit arch).

vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 7, 2024
    
We need to remove this function from cypari2, because pari 2.17 has
changed precision from words to bits which would cause confusion.

Besides, the current usage is incorrect, since `log_pari.precision()`
will give the precision in *decimal digits* not in words.

See: sagemath#38749

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
    
URL: sagemath#38908
Reported by: Gonzalo Tornaría
Reviewer(s): Vincent Delecroix
vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 8, 2024
    
We need to remove this function from cypari2, because pari 2.17 has
changed precision from words to bits which would cause confusion.

Besides, the current usage is incorrect, since `log_pari.precision()`
will give the precision in *decimal digits* not in words.

See: sagemath#38749

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
    
URL: sagemath#38908
Reported by: Gonzalo Tornaría
Reviewer(s): Vincent Delecroix
@tornaria
Copy link
Contributor

tornaria commented Nov 8, 2024

More updates in antonio-rojas#4

vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 9, 2024
    
We need to remove this function from cypari2, because pari 2.17 has
changed precision from words to bits which would cause confusion.

Besides, the current usage is incorrect, since `log_pari.precision()`
will give the precision in *decimal digits* not in words.

See: sagemath#38749

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
    
URL: sagemath#38908
Reported by: Gonzalo Tornaría
Reviewer(s): Vincent Delecroix
vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 13, 2024
    
We need to remove this function from cypari2, because pari 2.17 has
changed precision from words to bits which would cause confusion.

Besides, the current usage is incorrect, since `log_pari.precision()`
will give the precision in *decimal digits* not in words.

See: sagemath#38749

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
    
URL: sagemath#38908
Reported by: Gonzalo Tornaría
Reviewer(s): Vincent Delecroix
vbraun pushed a commit to vbraun/sage that referenced this pull request Nov 14, 2024
    
We need to remove this function from cypari2, because pari 2.17 has
changed precision from words to bits which would cause confusion.

Besides, the current usage is incorrect, since `log_pari.precision()`
will give the precision in *decimal digits* not in words.

See: sagemath#38749

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
    
URL: sagemath#38908
Reported by: Gonzalo Tornaría
Reviewer(s): Vincent Delecroix
@tornaria
Copy link
Contributor

See: sagemath/cypari2#165 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants