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)

4096-doc_const.patch (916 bytes) - added by AlexGhitza 11 years ago.
doc patch, apply to 3.1.2
4096-pari_real_precision_all.patch (165.4 KB) - added by AlexGhitza 11 years ago.
apply to 3.1.3.alpha1
trac_4096-pari_real_precision_all.patch (165.4 KB) - added by mabshoff 11 years ago.
This is the patch that was actually merged in 3.1.3.a2. It is slightly rebased against the previous patch

Download all attachments as: .zip

Change History (16)

comment:1 Changed 11 years ago by cremona

  • Summary changed from pari precision interface to [with patches, not to be reviewed yet] pari precision interface

The story so far: these apply in succession to 3.1.2.rc2:

  • Patch 1 (John) [originally called 4064-ell_pari_precision-2.patch]
  • Patch 2 (Alex) [..........................................3......]
  • Patch 3 (John) Adjusted previous for 64-bit
  • Patch 4 (John) ... and readjusted for 32-bit

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!

comment:2 Changed 11 years ago by AlexGhitza

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 cremona

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 AlexGhitza

I've added another tiny patch which removes unnecessarily complicated code from converting to Pari in polynomial_element.pyx.

Changed 11 years ago by AlexGhitza

doc patch, apply to 3.1.2

comment:5 Changed 11 years ago by AlexGhitza

  • 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 mabshoff

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: Changed 11 years ago by AlexGhitza

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 cremona

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.

Changed 11 years ago by AlexGhitza

apply to 3.1.3.alpha1

comment:9 follow-up: Changed 11 years ago by AlexGhitza

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 cremona

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 mabshoff

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 mabshoff

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 mabshoff

  • 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 mabshoff

  • Resolution set to fixed
  • Status changed from new to closed

Merged in Sage 3.1.3.alpha2

Note: See TracTickets for help on using tickets.