Opened 6 years ago

Closed 5 years ago

#16428 closed enhancement (fixed)

Cleanup/reorganization of FLINT imports

Reported by: ohanar Owned by:
Priority: major Milestone: sage-6.4
Component: misc Keywords:
Cc: Merged in:
Authors: R. Andrew Ohana, Jeroen Demeyer Reviewers: William Stein, Jean-Pierre Flori
Report Upstream: N/A Work issues:
Branch: b69be0d (Commits) Commit: b69be0da26e43f9454e676867afb54de530d2a58
Dependencies: Stopgaps:

Description (last modified by jpflori)

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).

Change History (50)

comment:1 Changed 6 years ago by ohanar

  • Status changed from new to needs_review

comment:2 Changed 6 years ago by was

  • Reviewers set to William Stein

comment:3 Changed 6 years ago by git

  • Commit changed from 8393c454263ab7377b6366ce28e1433dd83a39f7 to 82a574e7bd0b63ba30aa660321b308b37843f717

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

comment:4 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:5 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:6 Changed 5 years ago by jdemeyer

  • Resolution set to invalid
  • Status changed from needs_work to closed

comment:7 Changed 5 years ago by jdemeyer

  • Resolution invalid deleted
  • Status changed from closed to new

comment:8 Changed 5 years ago by jdemeyer

  • Authors changed from R. Andrew Ohana to R. Andrew Ohana, Jeroen Demeyer
  • Description modified (diff)

comment:9 Changed 5 years ago by jdemeyer

  • Branch changed from u/ohanar/cleanup_flint_imports to u/jdemeyer/ticket/16428
  • Created changed from 06/03/14 08:36:12 to 06/03/14 08:36:12
  • Modified changed from 10/13/14 18:41:55 to 10/13/14 18:41:55

comment:10 Changed 5 years ago by git

  • Commit changed from 82a574e7bd0b63ba30aa660321b308b37843f717 to f771a7909b1f5e7bda6e78f7b678ad8426f3bda7

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

f771a79Cleanup/reorganization of FLINT imports

comment:11 Changed 5 years ago by git

  • Commit changed from f771a7909b1f5e7bda6e78f7b678ad8426f3bda7 to 1fd22377f76029ef7a29c031c594d248b85ac8ff

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

1fd2237Cleanup/reorganization of FLINT imports

comment:12 Changed 5 years ago by jpflori

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

comment:13 Changed 5 years ago by git

  • Commit changed from 1fd22377f76029ef7a29c031c594d248b85ac8ff to 76ea0ae50c42efe44652ec754c2169eadffd4eae

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

76ea0aeCleanup/reorganization of FLINT imports

comment:14 Changed 5 years ago by git

  • Commit changed from 76ea0ae50c42efe44652ec754c2169eadffd4eae to aa472fb051c3806dafc8f912aaac26aa07aad89a

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

aa472fbCleanup/reorganization of FLINT imports

comment:15 Changed 5 years ago by git

  • Commit changed from aa472fb051c3806dafc8f912aaac26aa07aad89a to 9fef9845598c097c2ff61ab7381ded8380e62571

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

9fef984Cleanup/reorganization of FLINT imports

comment:16 Changed 5 years ago by git

  • Commit changed from 9fef9845598c097c2ff61ab7381ded8380e62571 to 2a2ba6d9211b31c31ea9130e98b998166d050716

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

2a2ba6dCleanup/reorganization of FLINT imports

comment:17 Changed 5 years ago by git

  • Commit changed from 2a2ba6d9211b31c31ea9130e98b998166d050716 to 8cfcccbff770eb7995bba15f44f58f8469bc8a9a

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

8cfcccbCleanup/reorganization of FLINT imports

comment:18 Changed 5 years ago by git

  • Commit changed from 8cfcccbff770eb7995bba15f44f58f8469bc8a9a to 6d5a978d162579cfcfec59b1cf767144f437bd80

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

6d5a978Cleanup/reorganization of FLINT imports

comment:19 Changed 5 years ago by git

  • Commit changed from 6d5a978d162579cfcfec59b1cf767144f437bd80 to efa14fdfe84132d3aca1d4c95f87cba12b25a6b1

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

efa14fdCleanup/reorganization of FLINT imports

comment:20 Changed 5 years ago by git

  • Commit changed from efa14fdfe84132d3aca1d4c95f87cba12b25a6b1 to 99b03b41b485e768a759a862d5585b2b9625cbba

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

99b03b4Cleanup/reorganization of FLINT imports

comment:21 Changed 5 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Cleanup/reorganization of some c imports (mainly flint) to Cleanup/reorganization of FLINT imports

comment:22 Changed 5 years ago by git

  • Commit changed from 99b03b41b485e768a759a862d5585b2b9625cbba to 22938bfb4e763e12465c9e4466c9b65f61f6b8d0

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

22938bfCleanup/reorganization of FLINT imports

comment:23 Changed 5 years ago by jdemeyer

With the current patch, the Sage library compiles but I get

$ ./sage --python
Python 2.7.8 (default, Oct 13 2014, 21:11:06) 
[GCC 4.6.4] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import sage.all
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/src/sage-git/local/lib/python2.7/site-packages/sage/all.py", line 87, in <module>
    from sage.misc.all       import *         # takes a while
  File "/usr/local/src/sage-git/local/lib/python2.7/site-packages/sage/misc/all.py", line 89, in <module>
    from functional import (additive_order,
  File "/usr/local/src/sage-git/local/lib/python2.7/site-packages/sage/misc/functional.py", line 32, in <module>
    import sage.interfaces.expect
  File "/usr/local/src/sage-git/local/lib/python2.7/site-packages/sage/interfaces/expect.py", line 60, in <module>
    from sage.interfaces.interface import Interface, InterfaceElement, InterfaceFunction, InterfaceFunctionElement, AsciiArtString
  File "/usr/local/src/sage-git/local/lib/python2.7/site-packages/sage/interfaces/interface.py", line 42, in <module>
    from sage.structure.parent_base import ParentWithBase
  File "sage/structure/parent_base.pyx", line 1, in init sage.structure.parent_base (build/cythonized/sage/structure/parent_base.c:2134)
  File "sage/structure/parent_old.pyx", line 1, in init sage.structure.parent_old (build/cythonized/sage/structure/parent_old.c:8811)
  File "sage/structure/element.pxd", line 12, in init sage.structure.parent (build/cythonized/sage/structure/parent.c:28430)
  File "sage/structure/element.pyx", line 3096, in init sage.structure.element (build/cythonized/sage/structure/element.c:34908)
  File "sage/categories/action.pxd", line 6, in init sage.structure.coerce (build/cythonized/sage/structure/coerce.c:17114)
ImportError: dynamic module does not define init function (initaction)

comment:24 Changed 5 years ago by jpflori

Very nice error, I think I got the same issue with some of my code. It had something to do with cython compiled stuff.

comment:25 follow-up: Changed 5 years ago by jpflori

I'm not really found of moving everything to a "decl" subdir though I'm aware of the fmpz_poly files issue. I would slightly find it better to move the "Fmpz_poly" class stuff somewhere else and keep the real FLINT C/Cython glue directly in "sage/libs/flint", but I won't fight for such a solution or another one though.

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

Replying to jpflori:

I would slightly find it better to move the "Fmpz_poly" class stuff somewhere else and keep the real FLINT C/Cython glue directly in "sage/libs/flint"

Would you prefer to move the Fmpz_poly class and the fmpz_poly() declarations in the same .pxd file?

comment:27 Changed 5 years ago by jdemeyer

Anyway, my first concern now is having something which works.

comment:28 follow-up: Changed 5 years ago by jpflori

Sure, the fmpz_poly thing is a low priority issue (and no, I don't want to see it in the fmpz_poly.pxd file, actually I did something in my branch as you first did, i.e. add some suffix, you chose old, I chose class.)

comment:29 Changed 5 years ago by jdemeyer

Scary, the problem is fixed by ./sage -ba

comment:30 Changed 5 years ago by jpflori

This is not very helpful but when the same issue happened to me, my browser history tells me I had a look at:

and I also remember it kind of got away randomly. Maybe it was caused by old compiled files (a lot of files changed names or were moved...)

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

Replying to jpflori:

and no, I don't want to see it in the fmpz_poly.pxd file

Actually, why not? It's perhaps not what I would have designed initially, but maybe it's the best solution given the current situation (I don't like renaming Cython modules just for the sake of it).

comment:32 Changed 5 years ago by jpflori

Yeah, that might be the solution with the less side effects. Except that doing from sage.libs.fmpz_poly cimport * which should basically do nothing for a pxd file just being glue for a C header will now (potentially) pull a bunch of actual Cython dependencies.

comment:33 Changed 5 years ago by git

  • Commit changed from 22938bfb4e763e12465c9e4466c9b65f61f6b8d0 to fff00d4c4180d597b1a0a0f6d85f05fa3611c28f

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

fff00d4Cleanup/reorganization of FLINT imports

comment:34 Changed 5 years ago by git

  • Commit changed from fff00d4c4180d597b1a0a0f6d85f05fa3611c28f to ce226027dfea0ea4a3c5fabee069c70a826a9790

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

ce22602Cleanup/reorganization of FLINT imports

comment:35 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:36 follow-up: Changed 5 years ago by jpflori

  • Description modified (diff)

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?

comment:37 Changed 5 years ago by jpflori

  • Description modified (diff)

Oops...

comment:38 Changed 5 years ago by jdemeyer

  • Status changed from new to needs_review

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

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.

comment:40 Changed 5 years ago by jdemeyer

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

comment:41 follow-ups: Changed 5 years ago by jpflori

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.

comment:42 Changed 5 years ago by jpflori

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

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

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.

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

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.

comment:45 Changed 5 years ago by jpflori

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]

comment:46 Changed 5 years ago by jpflori

  • Branch changed from u/jdemeyer/ticket/16428 to u/jpflori/ticket/16428
  • Commit changed from ce226027dfea0ea4a3c5fabee069c70a826a9790 to b69be0da26e43f9454e676867afb54de530d2a58
  • Reviewers changed from William Stein to William Stein, Jean-Pierre Flori

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.

comment:47 Changed 5 years ago by jdemeyer

Sorry, I have no idea what to do (as reviewer) with http://git.sagemath.org/sage.git/commit/?id=fc177409e274f01611a4bb7ae71623ee48ce655d

The other 2 commits obviously make sense.

comment:48 Changed 5 years ago by jpflori

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

comment:49 Changed 5 years ago by jpflori

  • Status changed from needs_review to positive_review

So positive_review I assume.

comment:50 Changed 5 years ago by vbraun

  • Branch changed from u/jpflori/ticket/16428 to b69be0da26e43f9454e676867afb54de530d2a58
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.