Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Declare PARI finite field functions (FF_*), wrap ffgen() and ffinit() #14818

Closed
pjbruin opened this issue Jun 25, 2013 · 20 comments
Closed

Declare PARI finite field functions (FF_*), wrap ffgen() and ffinit() #14818

pjbruin opened this issue Jun 25, 2013 · 20 comments

Comments

@pjbruin
Copy link
Contributor

pjbruin commented Jun 25, 2013

To access the PARI library functions related to the t_FFELT type for finite fields (see #12142), these functions need to be declared in sage/libs/pari/decl.pxi. Also, wrappers for the functions ffgen() and ffinit() are missing.

I am attaching a patch, which will become a dependency of #12142.

Apply:

CC: @jpflori

Component: finite rings

Keywords: pari

Author: Peter Bruin

Reviewer: Jean-Pierre Flori

Merged: sage-5.12.beta0

Issue created by migration from https://trac.sagemath.org/ticket/14818

@pjbruin
Copy link
Contributor Author

pjbruin commented Jun 25, 2013

based on 5.11.beta3

@pjbruin
Copy link
Contributor Author

pjbruin commented Jun 25, 2013

comment:1

Attachment: trac14818-pari_FF_decl.patch.gz

@pjbruin

This comment has been minimized.

@jpflori
Copy link

jpflori commented Jul 8, 2013

Reviewer: Jean-Pierre Flori

@jpflori
Copy link

jpflori commented Jul 8, 2013

Work Issues: doctests

@jpflori
Copy link

jpflori commented Jul 8, 2013

comment:3

Please add some doctests.

I know this will be indirectly doctested through further tickets, but it would be nice to have some example directly in the docstrings of ffgen and ffinit.

@jpflori
Copy link

jpflori commented Jul 8, 2013

comment:4

There is also a problem with the docstring for ffinit which mentions a polynomial P.

And the doc avout the v variable is kind of obscure.
In the docstring for get_var it's mentioned it should be a string or -1.
Here it seems you suggest it can be an integer.
Indeed, the PARI ffinit functions wants a long and will treat negative input in a special way, but the point of the call to get_var is to convert the var's string to the correct integer.
So either skip the get_var call or correcte the docstring.
I think the second option is better, and is what you wanted, isn't it?

@jpflori
Copy link

jpflori commented Jul 8, 2013

Changed work issues from doctests to doctests, docstring

@jpflori
Copy link

jpflori commented Jul 11, 2013

Reviewer patch.

@jpflori

This comment has been minimized.

@jpflori
Copy link

jpflori commented Jul 11, 2013

Changed work issues from doctests, docstring to none

@jpflori
Copy link

jpflori commented Jul 11, 2013

comment:6

Attachment: trav_14818-reviewer.patch.gz

Just added simple doctests and corrected the doc, so let's consider this a reviewer patch and I give positive review to the real changes made by Peter.

@jpflori

This comment has been minimized.

@pjbruin
Copy link
Contributor Author

pjbruin commented Jul 11, 2013

comment:8

Thanks, I realise that I was really sloppy with the docstrings...

@jpflori
Copy link

jpflori commented Jul 11, 2013

Reviewer patch.

@jpflori
Copy link

jpflori commented Jul 11, 2013

comment:9

Attachment: trac_14818-reviewer.patch.gz

Correct and better named patch.

@jpflori

This comment has been minimized.

@jpflori
Copy link

jpflori commented Jul 12, 2013

comment:10

For the patchbot, apply only:

@pjbruin
Copy link
Contributor Author

pjbruin commented Jul 16, 2013

comment:11

The patchbot doesn't seem to get it:

Apply trac14818-pari_FF_decl.patch, trac_14818-reviewer.patch

@jdemeyer
Copy link

jdemeyer commented Aug 2, 2013

Merged: sage-5.12.beta0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants