Opened 3 years ago

Closed 3 years ago

#22193 closed enhancement (fixed)

Port cypari2 documentation as a standalone doctests

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-7.6
Component: packages: standard Keywords:
Cc: defeo, jdemeyer Merged in:
Authors: Vincent Delecroix, Luca De Feo Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 128030a (Commits) Commit: 128030ad25fc7eb86c3995cab3a399d672c5e6a4
Dependencies: #22319 Stopgaps:

Description (last modified by vdelecroix)

Since cypari2 is aimed to become an external module we should move the doctest of its documentation in the standalone file src/sage/libs/pari/tests.py.

Attachments (1)

pari_doctests.py (1.5 KB) - added by vdelecroix 3 years ago.

Download all attachments as: .zip

Change History (70)

comment:1 Changed 3 years ago by jdemeyer

We already have src/sage/libs/pari/tests.py. That would be a good place to put those tests.

comment:2 Changed 3 years ago by vdelecroix

  • Description modified (diff)

comment:3 Changed 3 years ago by vdelecroix

  • Authors set to Vincent Delecroix

doing that...

Changed 3 years ago by vdelecroix

comment:4 Changed 3 years ago by vdelecroix

I joined the script I used to generate the files.

comment:5 Changed 3 years ago by vdelecroix

  • Branch set to u/vdelecroix/22193
  • Commit set to 3f211e45626df9f6338542decb4ef0df484fd7ff
  • Status changed from new to needs_review

New commits:

3f211e422193: copy doctests from cypari2

comment:6 Changed 3 years ago by jdemeyer

I don't really like that this is automatic. I think we should manually figure out which doctests should be kept in cypari2 and which ones should be moved to Sage.

comment:7 Changed 3 years ago by jdemeyer

I think that the tests in convert.pyx don't really involve Sage at all (except for the preparser, so 2^64 should be changed to 2**64 for example).

comment:8 Changed 3 years ago by defeo

What's the status of this? Anyone working on it?

comment:9 Changed 3 years ago by defeo

  • Branch changed from u/vdelecroix/22193 to u/defeo/22193

comment:10 Changed 3 years ago by defeo

  • Authors changed from Vincent Delecroix to Vincent Delecroix, Luca De Feo
  • Commit changed from 3f211e45626df9f6338542decb4ef0df484fd7ff to b6f22af9012c3f0e67511fabccf65e519ab43050
  • Dependencies set to #22221 #22222 #22291
  • Status changed from needs_review to needs_work

It's been extremely painful, but I've manually gone through all docstrings. As Jeroen suspected, very few of them were Sage-specific, but most of them needed to be de-Saged.

I moved the few Sage-specific tests to test.py, and also duplicated there some tests that could have been done entirely in CyPari2, but that used to activate some code-paths in Sage that wouldn't be activated otherwise.

There's two notable bits of docstring I haven't touched yet:

  1. The "Guide to real precision" in the module docstring of pari_instance.pyx. This is very Sage-specific, and it's more than just a testsuite. Where shall we move it?
  1. The Gen.sage(), which of course has no place in CyPari?. Did we decide anything on subclassing Gen, though?

For the rest, I hopefully changed all ^ to ** at the right spots, and removed all dependencies on Sage (except for the sage: prompt and the import statements). If I forgot something, we will notice anyway when setting up tests outside of Sage. I also fear that some doctests may fail in Python3, but that should also be easy to fix.

In my doctest trip, I saw a lot of very archaic code, esp. in the number field and the elliptic curve methods. For example nfbasis has a very weird docstring, and takes a flag parameter that seems to be undocumented in PARI.


New commits:

41d47f7Fix dependency on PARI headers
1755068Merge commit '41d47f7a14d9844c3a030c234a2c5d4a8971944f' of trac.sagemath.org:sage into t/22193/port_cypary2_doctests
98869b4Remove _pari_instance global
c9c93dcMerge commit '98869b4f83de894281bf164016d7ef9a2f4b3227' of trac.sagemath.org:sage into t/22193/port_cypary2_doctests
b5f74b1Use longintrepr.pxd from Cython
c6d3236Merge commit 'b5f74b1a88203cbc1768e47a2ae2037a1b6f18d6' of trac.sagemath.org:sage into t/22193/port_cypary2_doctests
b6f22afMade docstrings in libs/cypari2 mostyl Sage-independent

comment:11 Changed 3 years ago by git

  • Commit changed from b6f22af9012c3f0e67511fabccf65e519ab43050 to 52e9f8c03f97b53406c869c521264399a5ccd7cb

Branch pushed to git repo; I updated commit sha1. New commits:

52e9f8cRemoved Sage-specific bits from guide to real precision

comment:12 Changed 3 years ago by defeo

  • Status changed from needs_work to needs_review

comment:13 follow-up: Changed 3 years ago by jdemeyer

Just had a quick look and it looks good on first sight. I need to check a bit better in order to give positive_review though.

Does the documentation from src/sage/libs/pari/__init__.py actually appear anywhere?

comment:14 in reply to: ↑ 13 ; follow-up: Changed 3 years ago by defeo

Does the documentation from src/sage/libs/pari/__init__.py actually appear anywhere?

I don't think so, but all documentation from cypari2 seems to have disappeared from the reference manual anyway. I thought that was a deliberate choice?

comment:15 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Please rebase to 7.6.beta3

comment:16 in reply to: ↑ 14 Changed 3 years ago by jdemeyer

Replying to defeo:

all documentation from cypari2 seems to have disappeared from the reference manual anyway.

The code for src/sage/libs/cypari2 will be removed from Sage anyway, so I don't consider it a problem that the doc is not in Sage anymore.

However, the doc for src/sage/libs/pari should remain in Sage. It is currently not.

comment:17 Changed 3 years ago by jdemeyer

  • Branch changed from u/defeo/22193 to u/jdemeyer/22193

comment:18 Changed 3 years ago by jdemeyer

  • Commit changed from 52e9f8c03f97b53406c869c521264399a5ccd7cb to 3cb08162bfbc1d5b64f8899bd455907e6ddec937

Rebased (and fixed a typo in the commit message).


New commits:

8608c94Made docstrings in libs/cypari2 mostly Sage-independent
3cb0816Removed Sage-specific bits from guide to real precision

comment:19 Changed 3 years ago by jdemeyer

  • Dependencies #22221 #22222 #22291 deleted
  • Reviewers set to Jeroen Demeyer

I'm having a look at the documentation right now.

comment:20 Changed 3 years ago by git

  • Commit changed from 3cb08162bfbc1d5b64f8899bd455907e6ddec937 to db744ba0f606e1fda2fa7cac94b64baafaaf1aa5

Branch pushed to git repo; I updated commit sha1. New commits:

db744baAdd sage.libs.pari to the documentation

comment:21 Changed 3 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:22 Changed 3 years ago by defeo

  • Status changed from needs_review to positive_review

Thanks for the rebase.

We could improve the documentation of sage.libs.pari, but that's probably best kept for another ticket.

comment:23 follow-up: Changed 3 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Some details...

  1. You forgot to remove one int() here:
    sage: int(2) ** pari(-5)
    
  1. Why did you remove this?
    sage: bnr = pari("K = bnfinit(x^4 - 4*x^2 + 1); bnrinit(K, 2*x)")
    sage: bnr.nf_get_pol()
    
  1. This sentence is no longer relevant:
    However, beware that ``pari(L)`` returns an absolute number field::
    
  1. This is a significant change:
    -            sage: two = RealField(256)(2)._pari_()
    +            sage: two = pari(2)
    

Instead, you could use

two = pari("2.0000000000000000000000000000000000000000000000000000000000000000000000000000")
  1. This is confusing since the 5/3 is not the fraction 5/3 in Python!
    sage: pari([-1, 5/3, -8.0]).vecmin()
    

You could keep the original test with quotes: pari("[1, -5/3, 8.0]").vecmin()

comment:24 in reply to: ↑ 23 ; follow-up: Changed 3 years ago by defeo

  1. You forgot to remove one int() here:
    sage: int(2) ** pari(-5)
    

This breaks in Sage, unless we positive review #22321. But I wanted to explore that ticket a bit more before setting it to needs review.

On the other hand, it doesn't hurt Python.

  1. Why did you remove this?
    sage: bnr = pari("K = bnfinit(x^4 - 4*x^2 + 1); bnrinit(K, 2*x)")
    sage: bnr.nf_get_pol()
    

I did not remove it. I changed it to

sage: x = pari('x')                                                                                                         
sage: K = (x**4 - 4*x**2 + 1).nfinit()
sage: K.nf_get_pol()                                                                                                        

which seems to be functionally equivalent and more concise. However, you know PARI better than me. If you think that's more appropriate, I can change it to

sage: x = pari('x')                                                                                                         
sage: K = (x**4 - 4*x**2 + 1).bnfinit().bnrinit(2*x)                                                                        
sage: K.nf_get_pol()                                                                                                        

comment:25 Changed 3 years ago by defeo

  • Branch changed from u/jdemeyer/22193 to u/defeo/22193

comment:26 Changed 3 years ago by defeo

  • Commit changed from db744ba0f606e1fda2fa7cac94b64baafaaf1aa5 to 3e4bc1c84add1fc78e8951ae3d531cc59e033c42

fixed the rest


New commits:

3e4bc1cRestored some doctests

comment:27 Changed 3 years ago by git

  • Commit changed from 3e4bc1c84add1fc78e8951ae3d531cc59e033c42 to 4efdc2d774abfb09d9cb7d8ecee168091356bc5b

Branch pushed to git repo; I updated commit sha1. New commits:

0348e00Implement __index__ for PARI Gen
50593d6Minor fixes to __index__
83965bdMerge branch 'u/jdemeyer/implement___index___for_pari_gen' of git://trac.sagemath.org/sage into t/22193/22193
4efdc2dRemoved one unnecessary conversion to int in doctest

comment:28 Changed 3 years ago by defeo

  • Dependencies set to #22319

Replying to defeo:

  1. You forgot to remove one int() here:
    sage: int(2) ** pari(-5)
    

This breaks in Sage, unless we positive review #22321.

Wait, no, I got confused: it just needs #22319, which is positive reviewed. Fixed now.

comment:29 in reply to: ↑ 24 ; follow-up: Changed 3 years ago by jdemeyer

Replying to defeo:

... which seems to be functionally equivalent

But the bnrinit() call is gone, so it's clearly a different test.

Why not keep the original test? It should work as written in CyPari2.

comment:30 in reply to: ↑ 29 ; follow-up: Changed 3 years ago by defeo

Replying to jdemeyer:

Replying to defeo:

... which seems to be functionally equivalent

But the bnrinit() call is gone, so it's clearly a different test.

Why not keep the original test? It should work as written in CyPari2.

Because examples in the documentation should show the proper way to do things, not the ugly one.

I admit I do not understand the reason for the call to bnrinit(), maybe you can elucidate.

We can put the old test in TESTS::, however, if you wish to keep it.

comment:31 in reply to: ↑ 30 Changed 3 years ago by jdemeyer

Replying to defeo:

I admit I do not understand the reason for the call to bnrinit(), maybe you can elucidate.

It shows that nf_get_pol() works when given a bnr (the output of bnrinit()) as input. So it is an important test which really should not be removed.

comment:32 Changed 3 years ago by git

  • Commit changed from 4efdc2d774abfb09d9cb7d8ecee168091356bc5b to 96ddcd3616f7c18b757a34af4a15241b0696439d

Branch pushed to git repo; I updated commit sha1. New commits:

96ddcd3Restored old example with bnr

comment:33 Changed 3 years ago by defeo

  • Status changed from needs_work to needs_review

what about this?

comment:34 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:35 Changed 3 years ago by vbraun

  • Branch changed from u/defeo/22193 to 96ddcd3616f7c18b757a34af4a15241b0696439d
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:36 Changed 3 years ago by vbraun

  • Commit 96ddcd3616f7c18b757a34af4a15241b0696439d deleted
  • Resolution fixed deleted
  • Status changed from closed to new

fails on 32-bit:

sage -t --long src/sage/libs/cypari2/pari_instance.pyx
3569**********************************************************************
3570File "src/sage/libs/cypari2/pari_instance.pyx", line 112, in sage.libs.cypari2.pari_instance
3571Failed example:
3572    pari(float(1.0)).sin(precision=180) - pari(1).sin(precision=180)
3573Expected:
3574    5.42101086242752 E-20
3575Got:
3576    5.42101086 E-20
3577**********************************************************************
35781 item had failures:
3579   1 of  31 in sage.libs.cypari2.pari_instance
3580    [151 tests, 1 failure, 0.33 s]

comment:37 Changed 3 years ago by defeo

  • Branch changed from 96ddcd3616f7c18b757a34af4a15241b0696439d to u/defeo/96ddcd3616f7c18b757a34af4a15241b0696439d

comment:38 Changed 3 years ago by defeo

  • Commit set to 27251c31be002d282b0b10d9cdef92d07ee1d9c1
  • Status changed from new to needs_review

New commits:

8608c94Made docstrings in libs/cypari2 mostly Sage-independent
3cb0816Removed Sage-specific bits from guide to real precision
db744baAdd sage.libs.pari to the documentation
3e4bc1cRestored some doctests
83965bdMerge branch 'u/jdemeyer/implement___index___for_pari_gen' of git://trac.sagemath.org/sage into t/22193/22193
4efdc2dRemoved one unnecessary conversion to int in doctest
96ddcd3Restored old example with bnr
27251c3Fixed doctests failing on 32 bits, and improved docs

comment:39 follow-up: Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

I think it would be better to keep the spirit of the original examples instead of inventing a pointless example like pari(float(1.0)).sin(precision=180) - pari(1).sin(precision=180) == 0. Remember that these doctests are meant as documentation and I don't see what you are documenting there...

comment:40 in reply to: ↑ 39 ; follow-ups: Changed 3 years ago by defeo

Replying to jdemeyer:

I think it would be better to keep the spirit of the original examples instead of inventing a pointless example like pari(float(1.0)).sin(precision=180) - pari(1).sin(precision=180) == 0. Remember that these doctests are meant as documentation and I don't see what you are documenting there...

The original documentation said

In the third case, the precision is determined only by the inexact
inputs and the ``precision`` argument is ignored::
                                                  
    sage: pari(1.0).sin(precision=180).sage()
    0.841470984807896507
    sage: pari(1.0).sin(precision=40).sage()
    0.841470984807896507
    sage: pari(RealField(100).one()).sin().sage()
    0.84147098480789650665250232163029899962

which has no equivalent in CyPari?. Would you find this clearer?

    sage: d = pari(float(1.0)).sin(precision=180); d
    0.8414709848078965067
    sage: d.bitprecision()                                 
    64

However this test is also architecture dependent.

Last edited 3 years ago by defeo (previous) (diff)

comment:41 in reply to: ↑ 40 Changed 3 years ago by defeo

@jdemeyer ping?

Replying to defeo:

Replying to jdemeyer: Would you find this clearer?

    sage: d = pari(float(1.0)).sin(precision=180); d
    0.8414709848078965067
    sage: d.bitprecision()                                 
    64

However this test is also architecture dependent.

comment:42 in reply to: ↑ 40 Changed 3 years ago by jdemeyer

Replying to defeo:

which has no equivalent in CyPari?.

I disagree with this.

There are plenty of ways to create high-precision numbers in CyPari?:

sage: n = pari("1.0000000000000000000000000000"); n.bitprecision()
128
sage: n = pari("localbitprec(100); 1.0"); n.bitprecision()
128
sage: n = pari(1.0).bitprecision(100); n.bitprecision()
128

Personally, I would prefer the third way.

So I think that you can mostly keep the old doctests, just replacing pari(RealField(100)(1.0)) by pari(1.0).bitprecision(100).

comment:43 follow-up: Changed 3 years ago by jdemeyer

Looking at the documentation of precision more carefully, I think you should actually restore larger parts of the docs. For example, the first part of "Internal representation and conversion between Sage and PARI" and all of "Output precision for printing" should be kept for CyPari2.

comment:44 in reply to: ↑ 43 ; follow-ups: Changed 3 years ago by defeo

Replying to jdemeyer:

For example, the first part of "Internal representation and conversion between Sage and PARI"

There's pretty much nothing that can be salvaged from this section: precision in PARI is in multiples of 32 or 64. That's it.

and all of "Output precision for printing" should be kept for CyPari2.

I think this section is completely misleading, and the more I try to understand, the less I get it. It talks about "printing" precision, set_real_precision seems to affect more than printing:

sage: pari.set_real_precision_bits(1000)
sage: a = pari('Pi')
sage: 
sage: pari.set_real_precision_bits(10)
sage: b = pari('Pi')
sage: a - b
-2.17 E-19
sage: pari.set_real_precision_bits(1000)
sage: a - b
-2.168404344971008868 E-19
sage: a - b.bitprecision(1000)
-5.016557612668332023557327080330757013833665769218359371379100137196517465788293201785191348671769335290615539044941776827464059187151888254971589729806147889444035537705104506961803557118902433406655387152435176621321683472879809085435143307653965138954567335178660639273975085872627541300 E-20

I think this whole part needs to be rewritten, not translated from the old docs. Problem is, I'm not sure I understand what it does.

comment:45 in reply to: ↑ 44 Changed 3 years ago by jdemeyer

Can you explain me what you don't understand?

I could certainly try to improve the docs, but then I need to know what's wrong with the current docs.

comment:46 in reply to: ↑ 44 Changed 3 years ago by jdemeyer

Replying to defeo:

There's pretty much nothing that can be salvaged from this section: precision in PARI is in multiples of 32 or 64. That's it.

...but that is a very important fact to mention which is certainly more than "pretty much nothing".

comment:47 in reply to: ↑ 44 Changed 3 years ago by jdemeyer

Replying to defeo:

I think this section is completely misleading, and the more I try to understand, the less I get it. It talks about "printing" precision, set_real_precision seems to affect more than printing:

I hope that "completely misleading" is an exaggeration. The sentence "The maximum number of digits when printing a PARI real can be set using the methods Pari.set_real_precision_bits or Pari.set_real_precision" is correct but it's true that set_real_precision_bits affects more than just printing. It also affects precision for numbers parsed using the string interface, which is documented in the "Input precision for function calls" section. So I wonder where your confusion comes from.

comment:48 follow-up: Changed 3 years ago by defeo

Sorry, I shouldn't be doing so many things in parallel. I'll try to frame my doubts better.

For one, I do not understand this:

sage: pi = pari('Pi')
sage: a = pari(float(1.0))
sage: pari.set_real_precision(500)
15
sage: pi
3.141592653589793239
sage: a
1.0000000000000000000
sage: pi.bitprecision(1000)
3.1415926535897932385128089594061862044327426701784133911132812500000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
sage: a.bitprecision(1000)
1.00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000

How does PARI decide what bits to add when calling bitprecision()? Why does Pi have junk digits before all the zeros, while 1.0 gets only added zeros?

comment:49 Changed 3 years ago by defeo

Then, there's the fact that I expect (too optimistically) Pi to be a symbolic quantity, i.e. that incrementing precision (with pi.bitprecision(1000)) would compute correctly the new digits instead of adding random junk.

That's probably asking too much from PARI, but I got bad habits from Sage :)

The same behavior from pari.pi() surprises me much less: when I see a function call, I understand that's a function supposed to compute π up to a fixed precision.

Anyway, not much we can do here: that's the way PARI it is.

comment:50 Changed 3 years ago by defeo

And, finally, when I read

The maximum number of digits when printing a PARI real can be set using the methods :meth:Pari.set_real_precision_bits or :meth:Pari.set_real_precision.

I understand that's the SOLE role of set_real_precision: controlling output precision. Then, when I go further down, I discover that it also controls input precision for strings.

I would find it clearer, if it was stated right away that set_real_precision controls input and output precision for strings.

comment:51 in reply to: ↑ 48 ; follow-up: Changed 3 years ago by jdemeyer

Replying to defeo:

Sorry, I shouldn't be doing so many things in parallel. I'll try to frame my doubts better.

For one, I do not understand this:

sage: pi = pari('Pi')
sage: a = pari(float(1.0))
sage: pari.set_real_precision(500)
15
sage: pi
3.141592653589793239
sage: a
1.0000000000000000000
sage: pi.bitprecision(1000)
3.1415926535897932385128089594061862044327426701784133911132812500000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
sage: a.bitprecision(1000)
1.00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000

How does PARI decide what bits to add when calling bitprecision()?

It just adds zero bits.

Why does Pi have junk digits before all the zeros, while 1.0 gets only added zeros?

You should not make any conclusions from the decimal representation of the number, in binary it's just zeros.

Replying to defeo:

Then, there's the fact that I expect (too optimistically) Pi to be a symbolic quantity, i.e. that incrementing precision (with pi.bitprecision(1000)) would compute correctly the new digits instead of adding random junk.

PARI doesn't really do symbolic computations. Pi does a computation returning a floating-point approximation of π.

The same behavior from pari.pi() surprises me much less: when I see a function call, I understand that's a function supposed to compute π up to a fixed precision.

Actually, Pi is a function call. This works:

gp> Pi()
%1 = 3.1415926535897932384626433832795028842

It's just that the GP parser turns Pi into Pi() to improve user-friendliness.

comment:52 in reply to: ↑ 51 Changed 3 years ago by defeo

Why does Pi have junk digits before all the zeros, while 1.0 gets only added zeros?

You should not make any conclusions from the decimal representation of the number, in binary it's just zeros.

Right. So I think

n = pari(1.0).bitprecision(100)

is giving bad advice to the user: it only gets the intended value by chance, and the doctstring of bitprecision does nothing to help the user guess what the result would be.

It's just that the GP parser turns Pi into Pi() to improve user-friendliness.

I call this user-hateness.

comment:53 Changed 3 years ago by jdemeyer

Feel free to complain to the PARI developers for anything that you don't like...

comment:54 Changed 3 years ago by jdemeyer

Luca, are you going to work on this, or should I?

comment:55 Changed 3 years ago by defeo

Sorry, I was busy teaching. I was meaning to finalize this, but if I don't make it by today, it will have to wait until Fri.

Of course, if you have time to donate to this, feel free to jump in.

comment:56 Changed 3 years ago by jdemeyer

OK, I will try to have a look today.

comment:57 Changed 3 years ago by jdemeyer

  • Branch changed from u/defeo/96ddcd3616f7c18b757a34af4a15241b0696439d to u/jdemeyer/96ddcd3616f7c18b757a34af4a15241b0696439d

comment:58 Changed 3 years ago by git

  • Commit changed from 27251c31be002d282b0b10d9cdef92d07ee1d9c1 to fb79fd9275b295ac10a3d57f7acff997879f41c6

Branch pushed to git repo; I updated commit sha1. New commits:

fb79fd9Update documentation about precision

comment:59 Changed 3 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:60 Changed 3 years ago by jdemeyer

I hope this helps...

comment:61 follow-up: Changed 3 years ago by defeo

  • Status changed from needs_review to positive_review

Sorry for the delay. It is clearer now, thanks for the patch.

Are you sure the doctests pass on 32-bits? This, for example:

sage: c = pari(1).sin(); c
0.841470984807897
sage: c.bitprecision()
64

comment:62 in reply to: ↑ 61 ; follow-up: Changed 3 years ago by defeo

Are you sure the doctests pass on 32-bits? This, for example:

sage: c = pari(1).sin(); c
0.841470984807897
sage: c.bitprecision()
64

Ok, went again through the examples, and this example is obviously not a problem. The only problematic tests are those that do pari(1.0): that's not arch dependent in Sage (because 1.0 is a real), but it is in Python.

Anyway, they are ok for now. We shall see when we bring this to cypari.

comment:63 in reply to: ↑ 62 Changed 3 years ago by jdemeyer

Replying to defeo:

The only problematic tests are those that do pari(1.0): that's not arch dependent in Sage (because 1.0 is a real), but it is in Python.

No, Python floats are the same on 32-bit and 64-bit. It's true that certain computations involving floats might give different results. But for a conversion, the results should be the same on 32-bit and 64-bit.

comment:64 Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work
sage -t --long src/sage/libs/cypari2/pari_instance.pyx
**********************************************************************
File "src/sage/libs/cypari2/pari_instance.pyx", line 91, in sage.libs.cypari2.pari_instance
Failed example:
    pari("Pi") - p
Expected:
    1.22514845490862 E-16
Got:
    1.225148455 E-16
**********************************************************************
File "src/sage/libs/cypari2/pari_instance.pyx", line 94, in sage.libs.cypari2.pari_instance
Failed example:
    pari("Pi") - p
Expected:
    1.22514845490862 E-16
Got:
    1.225148455 E-16
**********************************************************************
1 item had failures:
   2 of  54 in sage.libs.cypari2.pari_instance
    [174 tests, 2 failures, 0.22 s]

comment:65 Changed 3 years ago by jdemeyer

Sorry for that.

comment:66 Changed 3 years ago by git

  • Commit changed from fb79fd9275b295ac10a3d57f7acff997879f41c6 to 128030ad25fc7eb86c3995cab3a399d672c5e6a4

Branch pushed to git repo; I updated commit sha1. New commits:

128030aLess digits are shown on 32-bit

comment:67 Changed 3 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:68 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to positive_review

This fixes the 32-bit tests.

comment:69 Changed 3 years ago by vbraun

  • Branch changed from u/jdemeyer/96ddcd3616f7c18b757a34af4a15241b0696439d to 128030ad25fc7eb86c3995cab3a399d672c5e6a4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.