#26493 closed enhancement (fixed)
element_pari_ffelt: use PARI clones instead of deepcopy_to_python_heap()
Reported by:  jdemeyer  Owned by:  

Priority:  major  Milestone:  sage8.5 
Component:  interfaces  Keywords:  
Cc:  pbruin, vdelecroix, defeo  Merged in:  
Authors:  Jeroen Demeyer  Reviewers:  Peter Bruin 
Report Upstream:  N/A  Work issues:  
Branch:  ace35ca (Commits)  Commit:  ace35cae5b50ee9cae8b3144f0f78310e967e3b4 
Dependencies:  Stopgaps: 
Description (last modified by )
The PARI "clone" functionality is pretty close to what deepcopy_to_python_heap()
does. Since it's handling PARI objects anyway, it makes sense to use the PARI gclone()
functionality too.
That is what cypari2 will do starting from version 2.0, so for consistency element_pari_ffelt
should do that too.
We also add a few minor unrelated changes:
 The signature of
construct_from
is changed fromvoid ... except *
toint ... except 1
to have more efficient error checking.
 Redundant Cython typing such as
FiniteFieldElement_pari_ffelt self
is removed.
 The
__pari__
and_repr_
methods no longer callsig_on()
.
Change History (12)
comment:1 Changed 2 years ago by
 Branch set to u/jdemeyer/element_pari_ffelt__use_pari_clones_instead_of_deepcopy_to_python_heap__
comment:2 Changed 2 years ago by
 Commit set to 5c272ef71856c18f8c4a639e66972fafb7e80295
 Status changed from new to needs_review
comment:3 Changed 2 years ago by
 Cc pbruin vdelecroix defeo added
comment:4 Changed 2 years ago by
 Commit changed from 5c272ef71856c18f8c4a639e66972fafb7e80295 to 8ee5adae4eea0cf657f0e380916f2f8ee6bec61c
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
8ee5ada  Use PARI clones instead of deepcopy_to_python_heap()

comment:5 Changed 2 years ago by
 Description modified (diff)
comment:6 Changed 2 years ago by
 Commit changed from 8ee5adae4eea0cf657f0e380916f2f8ee6bec61c to fd22c67d7a65904d35c14b9b6a70219deae2a9d0
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
fd22c67  Use PARI clones instead of deepcopy_to_python_heap()

comment:7 Changed 2 years ago by
 Commit changed from fd22c67d7a65904d35c14b9b6a70219deae2a9d0 to ace35cae5b50ee9cae8b3144f0f78310e967e3b4
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
ace35ca  Use PARI clones instead of deepcopy_to_python_heap()

comment:8 Changed 2 years ago by
 Description modified (diff)
comment:9 followup: ↓ 10 Changed 2 years ago by
 Reviewers set to Peter Bruin
 Status changed from needs_review to positive_review
Looks good to me. I noticed you use gcloneref
rather than gclone
. This is of course faster in the case where the object is already on the heap, which as far as I can see only happens when copying an existing element or creating one from a compatible pari_gen
. It does seem to imply that copy
will make a new Python object containing a reference to the same PARI object, but I guess this is perfectly fine since copy
is not required to do a deep copy.
comment:10 in reply to: ↑ 9 Changed 2 years ago by
Replying to pbruin:
It does seem to imply that
copy
will make a new Python object containing a reference to the same PARI object
Exactly. But finite field elements are immutable (you cannot change them inplace, neither in PARI nor in Sage), so that should not be a problem.
comment:11 Changed 2 years ago by
 Branch changed from u/jdemeyer/element_pari_ffelt__use_pari_clones_instead_of_deepcopy_to_python_heap__ to ace35cae5b50ee9cae8b3144f0f78310e967e3b4
 Resolution set to fixed
 Status changed from positive_review to closed
comment:12 Changed 2 years ago by
 Milestone changed from sage8.4 to sage8.5
This should be retargeted for 8.5.
New commits:
Use PARI clones instead of deepcopy_to_python_heap()