Opened 11 years ago
Closed 11 years ago
#4096 closed defect (fixed)
[with patches, positive review] pari precision interface
Reported by: | cremona | Owned by: | cremona |
---|---|---|---|
Priority: | major | Milestone: | sage-3.1.3 |
Component: | basic arithmetic | Keywords: | |
Cc: | AlexGhitza | Merged in: | |
Authors: | Reviewers: | ||
Report Upstream: | Work issues: | ||
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
This is a follow-up from 4064. Alex Ghitza and I are doing a big job sorting out the interface with the pari library with respect to (real and complex) precision, where there is currently confusion leading to weird results when word-precision, bit-precision and decimal precision are being confused.
Attachments (3)
Change History (16)
comment:1 Changed 11 years ago by
- Summary changed from pari precision interface to [with patches, not to be reviewed yet] pari precision interface
comment:2 Changed 11 years ago by
There is a new patch 4096-pari_real_precision.patch which replaces the previous ones, is based on 3.1.2.rc3, and adds more stuff. Still not quite ready for review, but we're getting there.
comment:3 Changed 11 years ago by
The following should be applied to 3.1.2:
4096-pari_real_precision.patch [Alex] 4096-pari_real_precision64.patch [John after 64-bit testing] 4096-matrix_real_pari64.patch [Alex PLUS some extra 64-bit stuff from John] 4096-pari_real_precision32.patch [tiny extra from John in number_field]
All tests pass.
comment:4 Changed 11 years ago by
I've added another tiny patch which removes unnecessarily complicated code from converting to Pari in polynomial_element.pyx.
comment:5 Changed 11 years ago by
- Summary changed from [with patches, not to be reviewed yet] pari precision interface to [with patches, needs review] pari precision interface
John and I agree that it is time for this to be reviewed.
To make this easier, I have put everything into one patch 4096-pari_real_precision_all.patch, which applies to 3.1.2. There is also a small doc patch 4096-doc_const.patch which fixes a related issue in const.tex, and also applies to 3.1.2.
Note to the reviewer: it would be best to start by scrolling down in the main patch until you hit the top of gen.pyx; there we have inserted a doc section called "Guide to real precision and the Pari library", which documents the correct behavior which is implemented by the patch.
comment:6 Changed 11 years ago by
Hmm, I don't like the following change:
178 s = str(self) 179 428 import sage.libs.pari.gen_py 180 return sage.libs.pari.gen_py.pari, (s,) 429 return sage.libs.pari.gen_py.pari, (str(self),)
I am not 100% certain, but if s were a C type object the above would cause a leak. I have fixed similar issues over and over again in code all over Sage and I suspect that the reference count for "return str(foo)" might be broken somehow. I have zero prove of this, obviously, but I intent to dig deep one day.
Cheers,
Michael
comment:7 follow-up: ↓ 8 Changed 11 years ago by
Michael,
I will rebase the patch against 3.1.3.alpha1 very soon and fix the issue that you're pointing out.
Alex
comment:8 in reply to: ↑ 7 Changed 11 years ago by
Replying to AlexGhitza:
Michael,
I will rebase the patch against 3.1.3.alpha1 very soon and fix the issue that you're pointing out.
Alex
It was fine with alpha0. Thanks, Alex.
comment:9 follow-up: ↓ 10 Changed 11 years ago by
there were a couple of rejects against 3.1.3.alpha1, so i replaced the patch with a rebased one
comment:10 in reply to: ↑ 9 Changed 11 years ago by
Replying to AlexGhitza:
there were a couple of rejects against 3.1.3.alpha1, so i replaced the patch with a rebased one
NB the doc/const patch still needs to be applied separately.
comment:11 Changed 11 years ago by
4096-pari_real_precision_all.patch is bruising faster than a Georgia peach falling from a tree. I rebased it again against my current 3.1.3.alpha2 treee in two places (one whitespace, the other a printing issue in mpfr_real.pyx) and will attach it shortly. I am testing it right now and am inclined to just merge it since both John and Alex spend considerable time on this. If this patch causes problem you can blame me, but at least that way it is in :). If Craig gets around to review this before 3.1.3.final it would be great it he put patches on top of what I am about to post.
Cheers,
Michael
Changed 11 years ago by
This is the patch that was actually merged in 3.1.3.a2. It is slightly rebased against the previous patch
comment:12 Changed 11 years ago by
- Summary changed from [with patches, needs review] pari precision interface to [with patches, positive review] pari precision interface
I read over the patch and it looks good to me. I am certainly no expert, so this positive review should be taken with a grain of salt. Since the patch did bitrot twice and was written by two experts I merged it into 3.1.3.alpha2. Should anything come up during subsequent review please open a new ticket so we can deal with that problem. The situation with the patch is certainly much improved over the old situation, so I consider this a worthy tradeoff. This patch also fixes #4199 and all doctests pass which is the main reason I merged it.
Cheers,
Michael
comment:13 Changed 11 years ago by
- Resolution set to fixed
- Status changed from new to closed
Merged in Sage 3.1.3.alpha2
The story so far: these apply in succession to 3.1.2.rc2:
The number of places where we need separate doctest results for 32- and 64-bit is very reduced but not entirely. The main work remaining is to sort out the constructor pari(K) where K is a number field. I thought I had sorted out pari(E) to be identical for 32 and 64 but it is not quite there yet (for E an elliptic curve).
There is no reason for the patch names to ahve ell_ in them, this is absolutely not just about elliptic curves!