Opened 6 years ago
Closed 6 years ago
#21806 closed enhancement (fixed)
Allow multiple instances of the PariInstance object
Reported by:  Jeroen Demeyer  Owned by:  

Priority:  major  Milestone:  sage7.6 
Component:  interfaces  Keywords:  atelierpari2017 
Cc:  Luca De Feo  Merged in:  
Authors:  Luca De Feo, Jeroen Demeyer  Reviewers:  Vincent Delecroix, Luca De Feo 
Report Upstream:  N/A  Work issues:  
Branch:  b70450d (Commits, GitHub, GitLab)  Commit:  b70450d27cd0a39bad2311ad5851057823285320 
Dependencies:  #21820  Stopgaps: 
Description (last modified by )
Since the PariInstance
object is really just a callable namespace without state, it should be possible to create multiple instances of it.
The rationale for this is that when multiple python modules depend upon CyPari, each one must instantiate its own PariInstance
object, or possibly an object inheriting from it.
Since PariInstance
initializes the PARI library upon __init__
, we must be careful to do this initialization only once.
When PariInstance.__init__()
is called a second time with a size
argument which is larger than the current size, we should increase (but never decrease) the PARI stack size but otherwise do nothing.
Change History (26)
comment:1 Changed 6 years ago by
Dependencies:  → #21820 

comment:2 Changed 6 years ago by
Branch:  → u/defeo/allow_multiple_instances_of_the_pariinstance_object 

comment:3 Changed 6 years ago by
Authors:  → Luca De Feo 

Commit:  → 061467fe7198373a0e715b9fa6737a435d256c21 
Status:  new → needs_review 
comment:4 Changed 6 years ago by
Keywords:  atelierpari2017 added 

comment:5 Changed 6 years ago by
I don't understand clearly why we should allow multiple pari instances. Could it be precised in the ticket description? (we discussed it yesterday but I don't quite remember)
comment:7 Changed 6 years ago by
Reviewers:  → Vincent Delecroix 

Status:  needs_review → needs_work 
comment:8 Changed 6 years ago by
Commit:  061467fe7198373a0e715b9fa6737a435d256c21 → aa6e5dd5967d539c9588933cd598a5e3558bee09 

Branch pushed to git repo; I updated commit sha1. New commits:
aa6e5dd  Removed trailing withespaces

comment:9 Changed 6 years ago by
Description:  modified (diff) 

Status:  needs_work → needs_review 
Note that an alternate, reasonable, design would initialize the PARI library at module load, rather than in __init__
. However this ticket is not concerned with this.
With the current design, we have the (dubious) advantage of being able to call pari_close()
(via PariInstance._close()
, and then reinitialize PARI by creating a new PariInstance
object at leisure.
comment:10 Changed 6 years ago by
Description:  modified (diff) 

comment:11 followup: 13 Changed 6 years ago by
pari is initialized at module load since a global pari_instance
is created! Calling _close
is dangerous since this global instance is still accessible... we might want to document this bugfeature
comment:12 Changed 6 years ago by
Branch:  u/defeo/allow_multiple_instances_of_the_pariinstance_object → u/jdemeyer/allow_multiple_instances_of_the_pariinstance_object 

comment:13 Changed 6 years ago by
Authors:  Luca De Feo → Luca De Feo, Jeroen Demeyer 

Commit:  aa6e5dd5967d539c9588933cd598a5e3558bee09 → 9df4382bcd2588c8243df5dcbd8e36e2c42ba914 
Milestone:  sage7.5 → sage7.6 
Replying to vdelecroix:
pari is initialized at module load since a global
pari_instance
is created!
But that will change. cypari2
should not do that.
New commits:
9df4382  Move initialization of PARI to PariInstance.__cinit__

comment:14 Changed 6 years ago by
Commit:  9df4382bcd2588c8243df5dcbd8e36e2c42ba914 → c2d09af38425f42aa438fc8d724d01149d967e23 

Branch pushed to git repo; I updated commit sha1. New commits:
c2d09af  Improve documentation of _close()

comment:15 Changed 6 years ago by
Commit:  c2d09af38425f42aa438fc8d724d01149d967e23 → 4b81781ebb2be6557cc9415ab614780cbbd25d7a 

Branch pushed to git repo; I updated commit sha1. New commits:
4b81781  Move _close() method right after __init__()

comment:17 Changed 6 years ago by
Branch:  u/jdemeyer/allow_multiple_instances_of_the_pariinstance_object → u/defeo/allow_multiple_instances_of_the_pariinstance_object 

comment:18 Changed 6 years ago by
Commit:  4b81781ebb2be6557cc9415ab614780cbbd25d7a → 80d40980013260f4925b5ca5898b2b674d23fefe 

Reviewers:  Vincent Delecroix → Vincent Delecroix, Luca De Feo 
comment:19 followup: 20 Changed 6 years ago by
What should I expect from
sage: from sage.libs.cypari2.pari_instance import PariInstance sage: p = PariInstance() sage: p._close() sage: p('12')
comment:20 followup: 21 Changed 6 years ago by
Replying to vdelecroix:
What should I expect from
sage: from sage.libs.cypari2.pari_instance import PariInstance sage: p = PariInstance() sage: p._close() sage: p('12')
A segmentation fault I guess.
comment:21 Changed 6 years ago by
Replying to jdemeyer:
Replying to vdelecroix:
What should I expect from
sage: from sage.libs.cypari2.pari_instance import PariInstance sage: p = PariInstance() sage: p._close() sage: p('12')A segmentation fault I guess.
Shouldn't it be documented as I mentioned in 11?
comment:22 followup: 23 Changed 6 years ago by
There is already the comment
Call this method at you own risk if you really need to free the memory used by PARI.
comment:23 Changed 6 years ago by
Replying to jdemeyer:
There is already the comment
Call this method at you own risk if you really need to free the memory used by PARI.
This comment is not clear enough. The sentence says that the memory is freed. Not that you will obtain a SEGFAULT
if you try to use pari again. Moreover, creating a new pari instance would make pari available again (right?).
comment:24 Changed 6 years ago by
Branch:  u/defeo/allow_multiple_instances_of_the_pariinstance_object → u/jdemeyer/allow_multiple_instances_of_the_pariinstance_object 

comment:25 Changed 6 years ago by
Commit:  80d40980013260f4925b5ca5898b2b674d23fefe → b70450d27cd0a39bad2311ad5851057823285320 

Status:  needs_review → positive_review 
New commits:
b70450d  Improve documentation of _close()

comment:26 Changed 6 years ago by
Branch:  u/jdemeyer/allow_multiple_instances_of_the_pariinstance_object → b70450d27cd0a39bad2311ad5851057823285320 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
Allow the PariInstance object to be allocated multiple times.