Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#26493 closed enhancement (fixed)

element_pari_ffelt: use PARI clones instead of deepcopy_to_python_heap()

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.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 jdemeyer)

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 from void ... except * to int ... 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 call sig_on().

Change History (12)

comment:1 Changed 2 years ago by jdemeyer

  • Branch set to u/jdemeyer/element_pari_ffelt__use_pari_clones_instead_of_deepcopy_to_python_heap__

comment:2 Changed 2 years ago by jdemeyer

  • Commit set to 5c272ef71856c18f8c4a639e66972fafb7e80295
  • Status changed from new to needs_review

New commits:

5c272efUse PARI clones instead of deepcopy_to_python_heap()

comment:3 Changed 2 years ago by jdemeyer

  • Cc pbruin vdelecroix defeo added

comment:4 Changed 2 years ago by git

  • Commit changed from 5c272ef71856c18f8c4a639e66972fafb7e80295 to 8ee5adae4eea0cf657f0e380916f2f8ee6bec61c

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

8ee5adaUse PARI clones instead of deepcopy_to_python_heap()

comment:5 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:6 Changed 2 years ago by git

  • Commit changed from 8ee5adae4eea0cf657f0e380916f2f8ee6bec61c to fd22c67d7a65904d35c14b9b6a70219deae2a9d0

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

fd22c67Use PARI clones instead of deepcopy_to_python_heap()

comment:7 Changed 2 years ago by git

  • Commit changed from fd22c67d7a65904d35c14b9b6a70219deae2a9d0 to ace35cae5b50ee9cae8b3144f0f78310e967e3b4

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

ace35caUse PARI clones instead of deepcopy_to_python_heap()

comment:8 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:9 follow-up: Changed 2 years ago by pbruin

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

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 in-place, neither in PARI nor in Sage), so that should not be a problem.

comment:11 Changed 2 years ago by vbraun

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

  • Milestone changed from sage-8.4 to sage-8.5

This should be re-targeted for 8.5.

Note: See TracTickets for help on using tickets.