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:  sage6.4 
Component:  misc  Keywords:  
Cc:  Merged in:  
Authors:  R. Andrew Ohana, Jeroen Demeyer  Reviewers:  William Stein, JeanPierre Flori 
Report Upstream:  N/A  Work issues:  
Branch:  b69be0d (Commits)  Commit:  b69be0da26e43f9454e676867afb54de530d2a58 
Dependencies:  Stopgaps: 
Description (last modified by )
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
 Status changed from new to needs_review
comment:2 Changed 6 years ago by
 Reviewers set to William Stein
comment:3 Changed 6 years ago by
 Commit changed from 8393c454263ab7377b6366ce28e1433dd83a39f7 to 82a574e7bd0b63ba30aa660321b308b37843f717
comment:4 Changed 5 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:5 Changed 5 years ago by
 Status changed from needs_review to needs_work
comment:6 Changed 5 years ago by
 Resolution set to invalid
 Status changed from needs_work to closed
comment:7 Changed 5 years ago by
 Resolution invalid deleted
 Status changed from closed to new
comment:8 Changed 5 years ago by
 Description modified (diff)
comment:9 Changed 5 years ago by
 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
 Commit changed from 82a574e7bd0b63ba30aa660321b308b37843f717 to f771a7909b1f5e7bda6e78f7b678ad8426f3bda7
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
f771a79  Cleanup/reorganization of FLINT imports

comment:11 Changed 5 years ago by
 Commit changed from f771a7909b1f5e7bda6e78f7b678ad8426f3bda7 to 1fd22377f76029ef7a29c031c594d248b85ac8ff
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
1fd2237  Cleanup/reorganization of FLINT imports

comment:12 Changed 5 years ago by
Argh, I basically have a branch doing the same thing...
comment:13 Changed 5 years ago by
 Commit changed from 1fd22377f76029ef7a29c031c594d248b85ac8ff to 76ea0ae50c42efe44652ec754c2169eadffd4eae
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
76ea0ae  Cleanup/reorganization of FLINT imports

comment:14 Changed 5 years ago by
 Commit changed from 76ea0ae50c42efe44652ec754c2169eadffd4eae to aa472fb051c3806dafc8f912aaac26aa07aad89a
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
aa472fb  Cleanup/reorganization of FLINT imports

comment:15 Changed 5 years ago by
 Commit changed from aa472fb051c3806dafc8f912aaac26aa07aad89a to 9fef9845598c097c2ff61ab7381ded8380e62571
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
9fef984  Cleanup/reorganization of FLINT imports

comment:16 Changed 5 years ago by
 Commit changed from 9fef9845598c097c2ff61ab7381ded8380e62571 to 2a2ba6d9211b31c31ea9130e98b998166d050716
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
2a2ba6d  Cleanup/reorganization of FLINT imports

comment:17 Changed 5 years ago by
 Commit changed from 2a2ba6d9211b31c31ea9130e98b998166d050716 to 8cfcccbff770eb7995bba15f44f58f8469bc8a9a
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
8cfcccb  Cleanup/reorganization of FLINT imports

comment:18 Changed 5 years ago by
 Commit changed from 8cfcccbff770eb7995bba15f44f58f8469bc8a9a to 6d5a978d162579cfcfec59b1cf767144f437bd80
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
6d5a978  Cleanup/reorganization of FLINT imports

comment:19 Changed 5 years ago by
 Commit changed from 6d5a978d162579cfcfec59b1cf767144f437bd80 to efa14fdfe84132d3aca1d4c95f87cba12b25a6b1
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
efa14fd  Cleanup/reorganization of FLINT imports

comment:20 Changed 5 years ago by
 Commit changed from efa14fdfe84132d3aca1d4c95f87cba12b25a6b1 to 99b03b41b485e768a759a862d5585b2b9625cbba
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
99b03b4  Cleanup/reorganization of FLINT imports

comment:21 Changed 5 years ago by
 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
 Commit changed from 99b03b41b485e768a759a862d5585b2b9625cbba to 22938bfb4e763e12465c9e4466c9b65f61f6b8d0
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
22938bf  Cleanup/reorganization of FLINT imports

comment:23 Changed 5 years ago by
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/sagegit/local/lib/python2.7/sitepackages/sage/all.py", line 87, in <module> from sage.misc.all import * # takes a while File "/usr/local/src/sagegit/local/lib/python2.7/sitepackages/sage/misc/all.py", line 89, in <module> from functional import (additive_order, File "/usr/local/src/sagegit/local/lib/python2.7/sitepackages/sage/misc/functional.py", line 32, in <module> import sage.interfaces.expect File "/usr/local/src/sagegit/local/lib/python2.7/sitepackages/sage/interfaces/expect.py", line 60, in <module> from sage.interfaces.interface import Interface, InterfaceElement, InterfaceFunction, InterfaceFunctionElement, AsciiArtString File "/usr/local/src/sagegit/local/lib/python2.7/sitepackages/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
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 followup: ↓ 26 Changed 5 years ago by
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
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
Anyway, my first concern now is having something which works.
comment:28 followup: ↓ 31 Changed 5 years ago by
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
Scary, the problem is fixed by ./sage ba
comment:30 Changed 5 years ago by
This is not very helpful but when the same issue happened to me, my browser history tells me I had a look at:
 http://stackoverflow.com/questions/8024805/cythoncompiledcextensionimporterrordynamicmoduledoesnotdefineinitfu
 http://stackoverflow.com/questions/24226001/importerrordynamicmoduledoesnotdefineinitfunctioninitfizzbuzz
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
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
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
 Commit changed from 22938bfb4e763e12465c9e4466c9b65f61f6b8d0 to fff00d4c4180d597b1a0a0f6d85f05fa3611c28f
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
fff00d4  Cleanup/reorganization of FLINT imports

comment:34 Changed 5 years ago by
 Commit changed from fff00d4c4180d597b1a0a0f6d85f05fa3611c28f to ce226027dfea0ea4a3c5fabee069c70a826a9790
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
ce22602  Cleanup/reorganization of FLINT imports

comment:35 Changed 5 years ago by
 Description modified (diff)
comment:36 followup: ↓ 39 Changed 5 years ago by
 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:38 Changed 5 years ago by
 Status changed from new to needs_review
comment:39 in reply to: ↑ 36 Changed 5 years ago by
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
Eventually the whole file gmp.pxi
(and other similar ones) should be gone.
comment:41 followups: ↓ 43 ↓ 44 Changed 5 years ago by
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
In fact the array stuff was already misplaced before...
comment:43 in reply to: ↑ 41 Changed 5 years ago by
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
Replying to jpflori:
The array things are in the wrong places in the definition of the
nmod...
types intypes.pxd
.
Please clarify (or post a reviewer patch), I have no idea what you mean.
comment:45 Changed 5 years ago by
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
 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, JeanPierre Flori
comment:47 Changed 5 years ago by
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
Oops.
In fact mpir
should read sage6.4.beta6
and I just merged your branch into it...
comment:49 Changed 5 years ago by
 Status changed from needs_review to positive_review
So positive_review I assume.
comment:50 Changed 5 years ago by
 Branch changed from u/jpflori/ticket/16428 to b69be0da26e43f9454e676867afb54de530d2a58
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Merge remotetracking branch 'trac/develop' into cleanup_flint_imports
fix new case of using __mpz_struct internals