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: sage-7.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:

Status badges

Description (last modified by Luca De Feo)

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 Jeroen Demeyer

Dependencies: #21820

comment:2 Changed 6 years ago by Luca De Feo

Branch: u/defeo/allow_multiple_instances_of_the_pariinstance_object

comment:3 Changed 6 years ago by Luca De Feo

Authors: Luca De Feo
Commit: 061467fe7198373a0e715b9fa6737a435d256c21
Status: newneeds_review

New commits:

061467fAllow the PariInstance object to be allocated multiple times.

comment:4 Changed 6 years ago by Luca De Feo

Keywords: atelierpari2017 added

comment:5 Changed 6 years ago by Vincent Delecroix

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:6 Changed 6 years ago by Vincent Delecroix

minor remark: there are trailing whitespaces!

comment:7 Changed 6 years ago by Vincent Delecroix

Reviewers: Vincent Delecroix
Status: needs_reviewneeds_work

comment:8 Changed 6 years ago by git

Commit: 061467fe7198373a0e715b9fa6737a435d256c21aa6e5dd5967d539c9588933cd598a5e3558bee09

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

aa6e5ddRemoved trailing withespaces

comment:9 Changed 6 years ago by Luca De Feo

Description: modified (diff)
Status: needs_workneeds_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 Luca De Feo

Description: modified (diff)

comment:11 Changed 6 years ago by Vincent Delecroix

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 bug-feature

comment:12 Changed 6 years ago by Jeroen Demeyer

Branch: u/defeo/allow_multiple_instances_of_the_pariinstance_objectu/jdemeyer/allow_multiple_instances_of_the_pariinstance_object

comment:13 in reply to:  11 Changed 6 years ago by Jeroen Demeyer

Authors: Luca De FeoLuca De Feo, Jeroen Demeyer
Commit: aa6e5dd5967d539c9588933cd598a5e3558bee099df4382bcd2588c8243df5dcbd8e36e2c42ba914
Milestone: sage-7.5sage-7.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:

9df4382Move initialization of PARI to PariInstance.__cinit__

comment:14 Changed 6 years ago by git

Commit: 9df4382bcd2588c8243df5dcbd8e36e2c42ba914c2d09af38425f42aa438fc8d724d01149d967e23

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

c2d09afImprove documentation of _close()

comment:15 Changed 6 years ago by git

Commit: c2d09af38425f42aa438fc8d724d01149d967e234b81781ebb2be6557cc9415ab614780cbbd25d7a

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

4b81781Move _close() method right after __init__()

comment:16 Changed 6 years ago by Jeroen Demeyer

New version, needs review...

comment:17 Changed 6 years ago by Luca De Feo

Branch: u/jdemeyer/allow_multiple_instances_of_the_pariinstance_objectu/defeo/allow_multiple_instances_of_the_pariinstance_object

comment:18 Changed 6 years ago by Luca De Feo

Commit: 4b81781ebb2be6557cc9415ab614780cbbd25d7a80d40980013260f4925b5ca5898b2b674d23fefe
Reviewers: Vincent DelecroixVincent Delecroix, Luca De Feo

All doctests pass, and I like Jeroens modifications.


New commits:

80d4098Typo

comment:19 Changed 6 years ago by Vincent Delecroix

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 in reply to:  19 ; Changed 6 years ago by Jeroen Demeyer

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 in reply to:  20 Changed 6 years ago by Vincent Delecroix

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 Changed 6 years ago by Jeroen Demeyer

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 in reply to:  22 Changed 6 years ago by Vincent Delecroix

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 Jeroen Demeyer

Branch: u/defeo/allow_multiple_instances_of_the_pariinstance_objectu/jdemeyer/allow_multiple_instances_of_the_pariinstance_object

comment:25 Changed 6 years ago by Vincent Delecroix

Commit: 80d40980013260f4925b5ca5898b2b674d23fefeb70450d27cd0a39bad2311ad5851057823285320
Status: needs_reviewpositive_review

New commits:

b70450dImprove documentation of _close()

comment:26 Changed 6 years ago by Volker Braun

Branch: u/jdemeyer/allow_multiple_instances_of_the_pariinstance_objectb70450d27cd0a39bad2311ad5851057823285320
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.