Opened 9 years ago

Closed 9 years ago

#15185 closed enhancement (fixed)

Clean up interface to the PARI library

Reported by: pbruin Owned by:
Priority: major Milestone: sage-6.1
Component: interfaces Keywords: pari
Cc: Merged in:
Authors: Peter Bruin Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: u/jdemeyer/ticket/15185 (Commits, GitHub, GitLab) Commit: 7ec74e0598300f96d1c41604fb288f41db4cc9da
Dependencies: #9640, #10018, #11868 Stopgaps:

Status badges

Description (last modified by pbruin)

The file sage/libs/pari/gen.pyx is too big and contains too many different things. For clarity and maintainability, it should be split into two parts:

  • gen.pyx containing the class gen;
  • pari_instance.pyx containing the class PariInstance and some utility functions;

More cleaning-up will be done in #15461.

Change History (22)

comment:1 Changed 9 years ago by jdemeyer

The t0GEN stuff is #11868.

comment:2 Changed 9 years ago by pbruin

  • Branch set to u/pbruin/15185
  • Created changed from 09/11/13 14:35:35 to 09/11/13 14:35:35
  • Modified changed from 10/03/13 10:24:44 to 10/03/13 10:24:44

comment:3 Changed 9 years ago by pbruin

  • Commit set to 2344f0bd14b5f72eb84ad1b134f84a74de25fc2e
  • Dependencies changed from #9640 to #9640 #10018
  • Work issues set to wait for #10018

comment:4 Changed 9 years ago by pbruin

  • Description modified (diff)

comment:5 Changed 9 years ago by pbruin

  • Authors set to Peter Bruin
  • Dependencies changed from #9640 #10018 to #9640, #10018

comment:6 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-5.13 to sage-6.0

comment:7 follow-up: Changed 9 years ago by jdemeyer

I think this ticket is a typical "patchbomb" which should be avoided. In particular, the replacing of the global variables t0... and the re-organizing of the .py(x) files are both big changes which should be on different tickets. For the former, I already created #11868 for this.

comment:8 in reply to: ↑ 7 Changed 9 years ago by pbruin

Replying to jdemeyer:

I think this ticket is a typical "patchbomb" which should be avoided.

I know. The problem is that when I started this clean-up operation, it wasn't clear what the end result was going to look like. I created this ticket -- and the list of separate issues -- only after reaching a state that worked again, and didn't know about #11868. Cutting it up into separate tickets is easier said than done; it will probably become more manageable by using Git, but it will still take a lot of effort.

comment:9 Changed 9 years ago by pbruin

  • Dependencies changed from #9640, #10018 to #9640, #10018, #11868
  • Description modified (diff)
  • Work issues changed from wait for #10018 to rebase

As a first step I split off the changes eliminating the global GEN variables as a patch on #11868. This will certainly have to be rebased, but that had to be done anyway because of #10018.

comment:10 follow-up: Changed 9 years ago by jdemeyer

To ease rebasing and reviewing, I recommend you to make this ticket absolutely minimal (only move stuff, nothing else): everything which can be done on a different ticket, should be done so.

comment:11 in reply to: ↑ 10 Changed 9 years ago by pbruin

Replying to jdemeyer:

To ease rebasing and reviewing, I recommend you to make this ticket absolutely minimal (only move stuff, nothing else): everything which can be done on a different ticket, should be done so.

That is more or less my plan, after #11868 is finished.

comment:12 Changed 9 years ago by pbruin

  • Description modified (diff)

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

Are we sure we really want to do this? I'm afraid that performance is going to be worse because Cython has an overhead when calling functions in a different module.

comment:14 in reply to: ↑ 13 Changed 9 years ago by pbruin

Replying to jdemeyer:

Are we sure we really want to do this? I'm afraid that performance is going to be worse because Cython has an overhead when calling functions in a different module.

Is this because the @cython.final decorator on an extension type T currently does not forbid subtyping T in a different Cython module (so that methods can be called directly in the same module, but only via a vtab in a different module)?

If this is what you mean, could the overhead be avoided by making new_gen() and clear_stack() (which will be responsible for most of the cross-module calls) functions instead of methods?

comment:15 Changed 9 years ago by pbruin

  • Branch changed from u/pbruin/15185 to u/pbruin/15185-split_up_pari_interface
  • Modified changed from 12/08/13 16:23:30 to 12/08/13 16:23:30

comment:16 Changed 9 years ago by pbruin

  • Commit changed from 2344f0bd14b5f72eb84ad1b134f84a74de25fc2e to 11a3bfbda21bde9c0d0778442cdd3607db14e583
  • Description modified (diff)
  • Status changed from new to needs_review
  • Work issues rebase deleted

Here is a new branch which moves PariInstance to a separate file and otherwise is close to minimal.

It is true that the Cython-generated code has some overhead in for the cross-module method calls. I think it is very small, but I will try to do some timings. Depending on the outcome, it might be reasonable to duplicate a small amount of code to avoid these cross-module calls. I think it is better to do that as part of #15461, though.


New commits:

11a3bfbmore fixes related to relocation of PariInstance?
34013ccfixes in sage.libs.pari
db8cd7etop-level changes related to PariInstance?
fcf7972adapt miscellaneous files in the Sage library
5091563fix imports in gen_py.py
478dd90move code from gen.pyx to pari_instance.pyx
08a278fcreate pari_instance.pyx; move some documentation and examples there
561e39fcreate pari_instance.pxd

comment:17 Changed 9 years ago by git

  • Commit changed from 11a3bfbda21bde9c0d0778442cdd3607db14e583 to 95a622a9843d9f99f7fa8009babb86ac27e2b32b

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

95a622abetter fix for real_double.pyx

comment:18 Changed 9 years ago by jdemeyer

  • Branch changed from u/pbruin/15185-split_up_pari_interface to u/jdemeyer/ticket/15185
  • Modified changed from 12/11/13 02:12:23 to 12/11/13 02:12:23

comment:19 Changed 9 years ago by jdemeyer

  • Commit changed from 95a622a9843d9f99f7fa8009babb86ac27e2b32b to 7ec74e0598300f96d1c41604fb288f41db4cc9da
  • Reviewers set to Jeroen Demeyer

Additional commit needs review. Apart from this, everything looks fine.


New commits:

7ec74e0PARI cleanup: reviewer patch

comment:20 Changed 9 years ago by pbruin

  • Status changed from needs_review to positive_review

I wasn't sure whether allocatemem() should already be removed from the global namespace (it was only deprecated recently), and whether from sage.libs.pari.all import ... is better than specifying the precise module, but I'm definitely OK with these changes.

comment:21 Changed 9 years ago by vbraun_spam

  • Milestone changed from sage-6.0 to sage-6.1

comment:22 Changed 9 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.