Opened 6 years ago
Closed 6 years ago
#22193 closed enhancement (fixed)
Port cypari2 documentation as a standalone doctests
Reported by:  Vincent Delecroix  Owned by:  

Priority:  major  Milestone:  sage7.6 
Component:  packages: standard  Keywords:  
Cc:  Luca De Feo, Jeroen Demeyer  Merged in:  
Authors:  Vincent Delecroix, Luca De Feo  Reviewers:  Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  128030a (Commits, GitHub, GitLab)  Commit:  128030ad25fc7eb86c3995cab3a399d672c5e6a4 
Dependencies:  #22319  Stopgaps: 
Description (last modified by )
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)
Change History (70)
comment:1 Changed 6 years ago by
comment:2 Changed 6 years ago by
Description:  modified (diff) 

Changed 6 years ago by
Attachment:  pari_doctests.py added 

comment:5 Changed 6 years ago by
Branch:  → u/vdelecroix/22193 

Commit:  → 3f211e45626df9f6338542decb4ef0df484fd7ff 
Status:  new → needs_review 
New commits:
3f211e4  22193: copy doctests from cypari2

comment:6 Changed 6 years ago by
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 6 years ago by
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:9 Changed 6 years ago by
Branch:  u/vdelecroix/22193 → u/defeo/22193 

comment:10 Changed 6 years ago by
Authors:  Vincent Delecroix → Vincent Delecroix, Luca De Feo 

Commit:  3f211e45626df9f6338542decb4ef0df484fd7ff → b6f22af9012c3f0e67511fabccf65e519ab43050 
Dependencies:  → #22221 #22222 #22291 
Status:  needs_review → needs_work 
It's been extremely painful, but I've manually gone through all docstrings. As Jeroen suspected, very few of them were Sagespecific, but most of them needed to be deSaged.
I moved the few Sagespecific tests to test.py
, and also duplicated there some tests that could have been done entirely in CyPari2, but that used to activate some codepaths in Sage that wouldn't be activated otherwise.
There's two notable bits of docstring I haven't touched yet:
 The "Guide to real precision" in the module docstring of
pari_instance.pyx
. This is very Sagespecific, and it's more than just a testsuite. Where shall we move it?
 The
Gen.sage()
, which of course has no place in CyPari?. Did we decide anything on subclassingGen
, 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:
41d47f7  Fix dependency on PARI headers

1755068  Merge commit '41d47f7a14d9844c3a030c234a2c5d4a8971944f' of trac.sagemath.org:sage into t/22193/port_cypary2_doctests

98869b4  Remove _pari_instance global

c9c93dc  Merge commit '98869b4f83de894281bf164016d7ef9a2f4b3227' of trac.sagemath.org:sage into t/22193/port_cypary2_doctests

b5f74b1  Use longintrepr.pxd from Cython

c6d3236  Merge commit 'b5f74b1a88203cbc1768e47a2ae2037a1b6f18d6' of trac.sagemath.org:sage into t/22193/port_cypary2_doctests

b6f22af  Made docstrings in libs/cypari2 mostyl Sageindependent

comment:11 Changed 6 years ago by
Commit:  b6f22af9012c3f0e67511fabccf65e519ab43050 → 52e9f8c03f97b53406c869c521264399a5ccd7cb 

Branch pushed to git repo; I updated commit sha1. New commits:
52e9f8c  Removed Sagespecific bits from guide to real precision

comment:12 Changed 6 years ago by
Status:  needs_work → needs_review 

comment:13 followup: 14 Changed 6 years ago by
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 followup: 16 Changed 6 years ago by
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:16 Changed 6 years ago by
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 6 years ago by
Branch:  u/defeo/22193 → u/jdemeyer/22193 

comment:18 Changed 6 years ago by
Commit:  52e9f8c03f97b53406c869c521264399a5ccd7cb → 3cb08162bfbc1d5b64f8899bd455907e6ddec937 

comment:19 Changed 6 years ago by
Dependencies:  #22221 #22222 #22291 

Reviewers:  → Jeroen Demeyer 
I'm having a look at the documentation right now.
comment:20 Changed 6 years ago by
Commit:  3cb08162bfbc1d5b64f8899bd455907e6ddec937 → db744ba0f606e1fda2fa7cac94b64baafaaf1aa5 

Branch pushed to git repo; I updated commit sha1. New commits:
db744ba  Add sage.libs.pari to the documentation

comment:21 Changed 6 years ago by
Status:  needs_work → needs_review 

comment:22 Changed 6 years ago by
Status:  needs_review → 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 followup: 24 Changed 6 years ago by
Status:  positive_review → needs_work 

Some details...
 You forgot to remove one
int()
here:sage: int(2) ** pari(5)
 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()
 This sentence is no longer relevant:
However, beware that ``pari(L)`` returns an absolute number field::
 This is a significant change:
 sage: two = RealField(256)(2)._pari_() + sage: two = pari(2)
Instead, you could use
two = pari("2.0000000000000000000000000000000000000000000000000000000000000000000000000000")
 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 followup: 29 Changed 6 years ago by
 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.
 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 6 years ago by
Branch:  u/jdemeyer/22193 → u/defeo/22193 

comment:26 Changed 6 years ago by
Commit:  db744ba0f606e1fda2fa7cac94b64baafaaf1aa5 → 3e4bc1c84add1fc78e8951ae3d531cc59e033c42 

comment:27 Changed 6 years ago by
Commit:  3e4bc1c84add1fc78e8951ae3d531cc59e033c42 → 4efdc2d774abfb09d9cb7d8ecee168091356bc5b 

Branch pushed to git repo; I updated commit sha1. New commits:
0348e00  Implement __index__ for PARI Gen

50593d6  Minor fixes to __index__

83965bd  Merge branch 'u/jdemeyer/implement___index___for_pari_gen' of git://trac.sagemath.org/sage into t/22193/22193

4efdc2d  Removed one unnecessary conversion to int in doctest

comment:28 Changed 6 years ago by
Dependencies:  → #22319 

comment:29 followup: 30 Changed 6 years ago by
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 followup: 31 Changed 6 years ago by
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 Changed 6 years ago by
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 6 years ago by
Commit:  4efdc2d774abfb09d9cb7d8ecee168091356bc5b → 96ddcd3616f7c18b757a34af4a15241b0696439d 

Branch pushed to git repo; I updated commit sha1. New commits:
96ddcd3  Restored old example with bnr

comment:34 Changed 6 years ago by
Status:  needs_review → positive_review 

comment:35 Changed 6 years ago by
Branch:  u/defeo/22193 → 96ddcd3616f7c18b757a34af4a15241b0696439d 

Resolution:  → fixed 
Status:  positive_review → closed 
comment:36 Changed 6 years ago by
Commit:  96ddcd3616f7c18b757a34af4a15241b0696439d 

Resolution:  fixed 
Status:  closed → new 
fails on 32bit:
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 E20 3575Got: 3576 5.42101086 E20 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 6 years ago by
Branch:  96ddcd3616f7c18b757a34af4a15241b0696439d → u/defeo/96ddcd3616f7c18b757a34af4a15241b0696439d 

comment:38 Changed 6 years ago by
Commit:  → 27251c31be002d282b0b10d9cdef92d07ee1d9c1 

Status:  new → needs_review 
New commits:
8608c94  Made docstrings in libs/cypari2 mostly Sageindependent

3cb0816  Removed Sagespecific bits from guide to real precision

db744ba  Add sage.libs.pari to the documentation

3e4bc1c  Restored some doctests

83965bd  Merge branch 'u/jdemeyer/implement___index___for_pari_gen' of git://trac.sagemath.org/sage into t/22193/22193

4efdc2d  Removed one unnecessary conversion to int in doctest

96ddcd3  Restored old example with bnr

27251c3  Fixed doctests failing on 32 bits, and improved docs

comment:39 followup: 40 Changed 6 years ago by
Status:  needs_review → 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 followups: 41 42 Changed 6 years ago by
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.
comment:41 Changed 6 years ago by
comment:42 Changed 6 years ago by
Replying to defeo:
which has no equivalent in CyPari?.
I disagree with this.
There are plenty of ways to create highprecision 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 followup: 44 Changed 6 years ago by
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 followups: 45 46 47 Changed 6 years ago by
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 E19 sage: pari.set_real_precision_bits(1000) sage: a  b 2.168404344971008868 E19 sage: a  b.bitprecision(1000) 5.016557612668332023557327080330757013833665769218359371379100137196517465788293201785191348671769335290615539044941776827464059187151888254971589729806147889444035537705104506961803557118902433406655387152435176621321683472879809085435143307653965138954567335178660639273975085872627541300 E20
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 Changed 6 years ago by
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 Changed 6 years ago by
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 Changed 6 years ago by
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 followup: 51 Changed 6 years ago by
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 6 years ago by
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 6 years ago by
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 followup: 52 Changed 6 years ago by
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.00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000How 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, while1.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 (withpi.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 floatingpoint 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 userfriendliness.
comment:52 Changed 6 years ago by
Why does
Pi
have junk digits before all the zeros, while1.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
intoPi()
to improve userfriendliness.
I call this userhateness.
comment:53 Changed 6 years ago by
Feel free to complain to the PARI developers for anything that you don't like...
comment:55 Changed 6 years ago by
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:57 Changed 6 years ago by
Branch:  u/defeo/96ddcd3616f7c18b757a34af4a15241b0696439d → u/jdemeyer/96ddcd3616f7c18b757a34af4a15241b0696439d 

comment:58 Changed 6 years ago by
Commit:  27251c31be002d282b0b10d9cdef92d07ee1d9c1 → fb79fd9275b295ac10a3d57f7acff997879f41c6 

Branch pushed to git repo; I updated commit sha1. New commits:
fb79fd9  Update documentation about precision

comment:59 Changed 6 years ago by
Status:  needs_work → needs_review 

comment:61 followup: 62 Changed 6 years ago by
Status:  needs_review → positive_review 

Sorry for the delay. It is clearer now, thanks for the patch.
Are you sure the doctests pass on 32bits? This, for example:
sage: c = pari(1).sin(); c 0.841470984807897 sage: c.bitprecision() 64
comment:62 followup: 63 Changed 6 years ago by
Are you sure the doctests pass on 32bits? 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 Changed 6 years ago by
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 32bit and 64bit. It's true that certain computations involving floats might give different results. But for a conversion, the results should be the same on 32bit and 64bit.
comment:64 Changed 6 years ago by
Status:  positive_review → 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 E16 Got: 1.225148455 E16 ********************************************************************** File "src/sage/libs/cypari2/pari_instance.pyx", line 94, in sage.libs.cypari2.pari_instance Failed example: pari("Pi")  p Expected: 1.22514845490862 E16 Got: 1.225148455 E16 ********************************************************************** 1 item had failures: 2 of 54 in sage.libs.cypari2.pari_instance [174 tests, 2 failures, 0.22 s]
comment:66 Changed 6 years ago by
Commit:  fb79fd9275b295ac10a3d57f7acff997879f41c6 → 128030ad25fc7eb86c3995cab3a399d672c5e6a4 

Branch pushed to git repo; I updated commit sha1. New commits:
128030a  Less digits are shown on 32bit

comment:67 Changed 6 years ago by
Status:  needs_work → needs_review 

comment:68 Changed 6 years ago by
Status:  needs_review → positive_review 

This fixes the 32bit tests.
comment:69 Changed 6 years ago by
Branch:  u/jdemeyer/96ddcd3616f7c18b757a34af4a15241b0696439d → 128030ad25fc7eb86c3995cab3a399d672c5e6a4 

Resolution:  → fixed 
Status:  positive_review → closed 
We already have
src/sage/libs/pari/tests.py
. That would be a good place to put those tests.