Opened 6 years ago

Closed 6 years ago

#18739 closed enhancement (fixed)

Wrap PARI functions for converting relative number field elements

Reported by: pbruin Owned by:
Priority: minor Milestone: sage-6.8
Component: interfaces Keywords: pari
Cc: Merged in:
Authors: Peter Bruin Reviewers: Frédéric Chapoton, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 5373b8e (Commits, GitHub, GitLab) Commit: 5373b8eeeeca1aef7816c85ccdf36d576850375d
Dependencies: Stopgaps:

Status badges

Description

Wrap the PARI library functions nf_rnfeq, eltabstorel, eltabstorel_lift and eltreltoabs. These provide a slightly lower-level interface to rnfeltabstorel and rnfeltreltoabs, and have the advantage that one can avoid initialising a full PARI rnf structure. This functionality will be used in a follow-up ticket.

Change History (13)

comment:1 Changed 6 years ago by pbruin

  • Branch set to u/pbruin/18739-pari_rnf_conversion
  • Commit set to 5055d694d750f40db8f931d12abcff0d74233ed5
  • Status changed from new to needs_review

comment:2 Changed 6 years ago by chapoton

  • Branch changed from u/pbruin/18739-pari_rnf_conversion to public/ticket/18739
  • Commit changed from 5055d694d750f40db8f931d12abcff0d74233ed5 to d27251de48beac243d5c3b1d8e6f4b607c375303
  • Reviewers set to Frédéric Chapoton

Looks good to me.

I have used this opportunity to

  • correct one typo in the doc (missing ::)
  • put all raise statements into python3 syntax.

I you agree with these cosmetic changes, you can set a positive review on my behalf.


New commits:

566770aMerge branch 'u/pbruin/18739-pari_rnf_conversion' into 6.8.b5
88fbd3dtrac #18379 doc typo in pari/gen
d27251dtrac #18739 raise into py3 syntax

comment:3 Changed 6 years ago by jdemeyer

  • Status changed from needs_review to needs_work

I think you can remove __xor__ completely, since it's inherited from Element.

comment:4 follow-up: Changed 6 years ago by jdemeyer

Those functions you are adding should not be part of the public interface of gen, they are unsafe:

sage: pari(0).eltabstorel(0)
...
SignalError: Segmentation fault

If you really want to add them to gen, add an underscore (_eltabstorel instead of eltabstorel) to the method name and add a WARNING in the documentation that these are unsafe.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 6 years ago by pbruin

Replying to jdemeyer:

Those functions you are adding should not be part of the public interface of gen, they are unsafe:

sage: pari(0).eltabstorel(0)
...
SignalError: Segmentation fault

I can change this, but there are more methods like this:

sage: pari(0).pr_get_e()
...
SignalError: Segmentation fault

Maybe we should at some point make a better separation between functions that are also in GP (and do extra input checking) and PARI library functions (that crash when given wrong input).

comment:6 in reply to: ↑ 5 Changed 6 years ago by jdemeyer

Replying to pbruin:

sage: pari(0).pr_get_e()
...
SignalError: Segmentation fault

I consider this a bug.

I think adding an underscore to all non-safe methods is an easy and good solution.

comment:7 Changed 6 years ago by git

  • Commit changed from d27251de48beac243d5c3b1d8e6f4b607c375303 to 5373b8eeeeca1aef7816c85ccdf36d576850375d

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

5373b8eTrac 18739: reviewer comments

comment:8 Changed 6 years ago by pbruin

  • Reviewers changed from Frédéric Chapoton to Frédéric Chapoton, Jeroen Demeyer
  • Status changed from needs_work to needs_review

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

  • Status changed from needs_review to needs_work

Please replace

This method may raise errors or
return undefined results if called with invalid arguments.

by

This method may segfault or
return undefined results if called with invalid arguments.

Modulo this, I give the ticket positive_review.

comment:10 in reply to: ↑ 9 ; follow-up: Changed 6 years ago by pbruin

Replying to jdemeyer:

Please replace

This method may raise errors or
return undefined results if called with invalid arguments.

by

This method may segfault or
return undefined results if called with invalid arguments.

I thought about phrasing it like that, but to me "segfault" (besides perhaps being somewhat cryptic to many users) makes it sound like Sage crashes if this is called with the wrong arguments. In fact, the segmentation fault is caught and a SignalError is raised. Also, it is entirely possible that PARI will throw other kinds of errors for certain inputs.

comment:11 Changed 6 years ago by pbruin

  • Status changed from needs_work to needs_review

After some further thought, I still think "raise errors" is better than "segfault". We could write a longer warning to explain what could happen on bad input, but I don't think it is worth the effort...

comment:12 in reply to: ↑ 10 Changed 6 years ago by jdemeyer

  • Status changed from needs_review to positive_review

I still don't agree (whether or not Sage catches the segfault, it's still a segfault which should be avoided at all costs), but I won't obstruct...

comment:13 Changed 6 years ago by vbraun

  • Branch changed from public/ticket/18739 to 5373b8eeeeca1aef7816c85ccdf36d576850375d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.