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

Cleanup/reorganization of FLINT imports #16428

Closed
ohanar opened this issue Jun 3, 2014 · 64 comments
Closed

Cleanup/reorganization of FLINT imports #16428

ohanar opened this issue Jun 3, 2014 · 64 comments

Comments

@ohanar
Copy link
Member

ohanar commented Jun 3, 2014

Rename the FLINT .pxi files to .pxd files. Create a new file src/sage/libs/flint/types.pxd with just the type declarations (similar to GMP, very useful to avoid circular imports).

Component: misc

Author: R. Andrew Ohana, Jeroen Demeyer

Branch/Commit: b69be0d

Reviewer: William Stein, Jean-Pierre Flori

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

@ohanar ohanar added this to the sage-6.3 milestone Jun 3, 2014
@williamstein
Copy link
Contributor

Reviewer: William Stein

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 7, 2014

Changed commit from 8393c45 to 82a574e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 7, 2014

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

acad84eMerge remote-tracking branch 'trac/develop' into cleanup_flint_imports
82a574efix new case of using __mpz_struct internals

@jdemeyer
Copy link

Changed author from R. Andrew Ohana to R. Andrew Ohana, Jeroen Demeyer

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link

Changed branch from u/ohanar/cleanup_flint_imports to u/jdemeyer/ticket/16428

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 13, 2014

Changed commit from 82a574e to f771a79

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 13, 2014

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

f771a79Cleanup/reorganization of FLINT imports

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 13, 2014

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

1fd2237Cleanup/reorganization of FLINT imports

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 13, 2014

Changed commit from f771a79 to 1fd2237

@jpflori
Copy link

jpflori commented Oct 13, 2014

comment:12

Argh, I basically have a branch doing the same thing...

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 13, 2014

Changed commit from 1fd2237 to 76ea0ae

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 13, 2014

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

76ea0aeCleanup/reorganization of FLINT imports

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 13, 2014

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

aa472fbCleanup/reorganization of FLINT imports

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 13, 2014

Changed commit from 76ea0ae to aa472fb

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 13, 2014

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

9fef984Cleanup/reorganization of FLINT imports

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 13, 2014

Changed commit from aa472fb to 9fef984

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 13, 2014

Changed commit from 9fef984 to 2a2ba6d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 13, 2014

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

2a2ba6dCleanup/reorganization of FLINT imports

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 14, 2014

Changed commit from fff00d4 to ce22602

@jdemeyer

This comment has been minimized.

@jpflori

This comment has been minimized.

@jpflori
Copy link

jpflori commented Oct 14, 2014

comment:36

I see some mpz_... functions defined in Sage files have been deleted.
I guess it's because FLINT now provides similar functionalities, or at least the functions using them in Sage have changed to use other flitn functions?

@jpflori
Copy link

jpflori commented Oct 14, 2014

comment:37

Oops...

@jpflori

This comment has been minimized.

@jdemeyer
Copy link

comment:39

Replying to @jpflori:

I see some mpz_... functions defined in Sage files have been deleted.

They were unused. Some of them are actually copied to a proper .pyx file where they belong.

@jdemeyer
Copy link

comment:40

Eventually the whole file gmp.pxi (and other similar ones) should be gone.

@jpflori
Copy link

jpflori commented Oct 14, 2014

comment:41

The array things are in the wrong places in the definition of the nmod... types in types.pxd. Apart from that it looks ok.

Could you correct that in a new commit (that is not doing a forced push).
I'm currently testing the branch in a personal branch using more stuff from flint and merging forced push can be a pain sometimes.

I'll restest everything on top of a vanilla trac/develop branch a little later.

@jpflori
Copy link

jpflori commented Oct 14, 2014

comment:42

In fact the array stuff was already misplaced before...

@jdemeyer
Copy link

comment:43

Replying to @jpflori:

I'm currently testing the branch in a personal branch using more stuff from flint and merging forced push can be a pain sometimes.

Concerning forced pushes: in my mind (this is not official policy), forced pushes are fine before setting a ticket to needs_review. The alternative is not pushing until you have something ready for review, which is worse.

@jdemeyer
Copy link

comment:44

Replying to @jpflori:

The array things are in the wrong places in the definition of the nmod... types in types.pxd.

Please clarify (or post a reviewer patch), I have no idea what you mean.

@jpflori
Copy link

jpflori commented Oct 15, 2014

comment:45

There are some

ctypedef nmod_poly_struct[1] nmod_poly_t

or variations and similar
which should be

ctypedef nmod_poly_struct nmod_poly_t[1]

@jpflori
Copy link

jpflori commented Oct 15, 2014

Changed commit from ce22602 to b69be0d

@jpflori
Copy link

jpflori commented Oct 15, 2014

comment:46

I'm basically happy with Jeroen work.

Jeroen: can you double check my latest changes?
If they suit you, lgtm.


New commits:

fc17740Merge remote-tracking branch 'trac/u/jdemeyer/ticket/16428' into mpir
a669ec7Fix test in cython.py for new FLINT layout.
b69be0dFix some ctypedef in FLINT pxd files.

@jpflori
Copy link

jpflori commented Oct 15, 2014

Changed reviewer from William Stein to William Stein, Jean-Pierre Flori

@jpflori
Copy link

jpflori commented Oct 15, 2014

Changed branch from u/jdemeyer/ticket/16428 to u/jpflori/ticket/16428

@jdemeyer
Copy link

comment:47

Sorry, I have no idea what to do (as reviewer) with sagemath/sagetrac-mirror@fc17740

The other 2 commits obviously make sense.

@jpflori
Copy link

jpflori commented Oct 15, 2014

comment:48

Oops.
In fact mpir should read sage-6.4.beta6 and I just merged your branch into it...

@jpflori
Copy link

jpflori commented Oct 15, 2014

comment:49

So positive_review I assume.

@vbraun
Copy link
Member

vbraun commented Oct 16, 2014

Changed branch from u/jpflori/ticket/16428 to b69be0d

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

5 participants