Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#16583 closed enhancement (fixed)

Clean up FLINT declarations

Reported by: malb Owned by:
Priority: major Milestone: sage-6.4
Component: basic arithmetic Keywords: sd59
Cc: Merged in:
Authors: Martin Albrecht Reviewers: Jeroen Demeyer, Volker Braun
Report Upstream: N/A Work issues:
Branch: 20c767c (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

Update fmpz.pxi and fmpz_poly.pxi (and others) so that Cython doesn't complain about referenced before assigment any more.

Change History (30)

comment:1 Changed 7 years ago by malb

  • Branch set to u/malb/t16583_fmpz_poly
  • Commit set to dfc8ce1e411eee34ce50f69f37fe37c73cbe1f79
  • Status changed from new to needs_review

New commits:

3bda21ccleanup of fmpz_t and fmpz_poly_t to avoid Cython warnings about referencing before assignment
dfc8ce1silence Cython warning about complicated declaration

comment:2 Changed 7 years ago by chapoton

  • Status changed from needs_review to needs_work

This needs to be rebased, there is a conflict..

comment:3 Changed 7 years ago by chapoton

and a typo in the second commit, by the way.

comment:4 Changed 7 years ago by git

  • Commit changed from dfc8ce1e411eee34ce50f69f37fe37c73cbe1f79 to 8bb6b48617d5e8cdf2a8f50f269332d591dcd8a7

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

8bb6b48merge

comment:5 Changed 7 years ago by malb

rebased & fixed typo

comment:6 Changed 7 years ago by chapoton

Is this related to #13532 ?

comment:7 Changed 7 years ago by malb

Yep, it removes some of those warnings. The big TODO is to fix mpz_t, though, which is a lot of work.

comment:8 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:9 Changed 6 years ago by chapoton

  • Branch changed from u/malb/t16583_fmpz_poly to public/ticket/16583
  • Commit changed from 8bb6b48617d5e8cdf2a8f50f269332d591dcd8a7 to 14e730e84c2df7b5ccbe4aca490ae8c78d9fa43d

I rebased on 6.4.beta1, hopefully in a correct way


New commits:

14e730eMerge with 6.4.beta1

comment:10 Changed 6 years ago by git

  • Commit changed from 14e730e84c2df7b5ccbe4aca490ae8c78d9fa43d to 18a48429757e11afd32818b8940114e83a511824

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

18a4842trac #16583, oops, correcting wrong merge

comment:11 Changed 6 years ago by malb

  • Status changed from needs_work to needs_review

I guess this should be 'needs review'. Your rebasing looks good.

comment:12 Changed 6 years ago by jdemeyer

In src/sage/libs/flint/fmpz_poly.pxd, what's the reason you remove the declaration for fmpz_poly_reverse but not fmpz_poly_revert_series? I think both can be removed.

comment:13 Changed 6 years ago by jdemeyer

  • Branch changed from public/ticket/16583 to u/jdemeyer/ticket/16583
  • Created changed from 06/28/14 17:14:18 to 06/28/14 17:14:18
  • Modified changed from 08/27/14 20:52:23 to 08/27/14 20:52:23

comment:14 Changed 6 years ago by jdemeyer

  • Commit changed from 18a48429757e11afd32818b8940114e83a511824 to c416fee312ff1d8ab43b3a3c650f44688a17c7b9

Looks good. If you agree with my reviewer patch, then it's positive_review for me.


New commits:

c416feeRemove stuff which is not needed

comment:15 Changed 6 years ago by malb

  • Status changed from needs_review to positive_review

looks good.

comment:16 Changed 6 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from positive_review to needs_work

Sorry, just found an issue. In fmpq_poly.pxd, there still is

    ctypedef void * fmpz_t
    ctypedef void * fmpq_poly_t

I'll create a patch.

comment:17 Changed 6 years ago by git

  • Commit changed from c416fee312ff1d8ab43b3a3c650f44688a17c7b9 to b3d6aa2c24defa7cd22675f5540659b07a25401c

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

b3d6aa2Further cleanup

comment:18 Changed 6 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:19 Changed 6 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from clean up fmpz_t and fmpz_poly_t declarations to Clean up FLINT declarations

comment:20 Changed 6 years ago by malb

  • Status changed from needs_review to positive_review

Looks good.

comment:21 Changed 6 years ago by vbraun

  • Status changed from positive_review to needs_work

Some doctests still import fmpq_poly.pxd:

sage -t --long src/sage/misc/cython.py
**********************************************************************
File "src/sage/misc/cython.py", line 254, in sage.misc.cython.pyx_preparse
Failed example:
    module = sage.misc.cython.import_test("trac11680")  # long time (7s on sage.math, 2012)
Exception raised:
    Traceback (most recent call last):
      File "/home/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 480, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 843, in compile_and_execute
        exec compiled in globs
      File "<doctest sage.misc.cython.pyx_preparse[9]>", line 1, in <module>
        module = sage.misc.cython.import_test("trac11680")  # long time (7s on sage.math, 2012)
      File "/home/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/misc/cython.py", line 805, in import_test
        return compile_and_load(TESTS[name])
      File "/home/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/misc/cython.py", line 757, in compile_and_load
        return cython_import(file, create_local_c_file=False)
      File "/home/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/server/support.py", line 447, in cython_import
        **kwds)
      File "/home/buildslave-sage/slave/sage_git/build/local/lib/python2.7/site-packages/sage/misc/cython.py", line 493, in cython
        raise RuntimeError("Error converting {} to C:\n{}\n{}".format(filename, log, err))
    RuntimeError: Error converting /home/buildslave-sage/slave/sage_git/dot_sage/temp/desktop/29631/tmp_i0rQYs.pyx to C:


    Error compiling Cython file:
    ------------------------------------------------------------
    ...
    #cinclude $SAGE_SRC/sage/libs/flint $SAGE_LOCAL/include/FLINT
    #clib flint

    from sage.rings.rational cimport Rational
    from sage.rings.polynomial.polynomial_rational_flint cimport Polynomial_rational_flint
    from sage.libs.flint.fmpq_poly cimport (fmpq_poly_get_coeff_mpq, fmpq_poly_set_coeff_mpq,
    ^
    ------------------------------------------------------------

    _home_buildslave_sage_slave_sage_git_dot_sage_temp_desktop_29631_tmp_i0rQYs_pyx_0.pyx:13:0: 'sage.libs.flint.fmpq_poly.pxd' not found

comment:22 Changed 6 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:23 Changed 6 years ago by git

  • Commit changed from b3d6aa2c24defa7cd22675f5540659b07a25401c to 20c767c6a446b441b1551a03a10fb09e1e4bb711

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

20c767cFix include of fmpq_poly.pxi in Cython doctest

comment:24 Changed 6 years ago by vbraun

  • Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Volker Braun
  • Status changed from needs_review to positive_review

comment:25 Changed 6 years ago by jdemeyer

I have also done the same fixes for mpz_t at #16910 + #15946, it would be good if somebody could review these.

comment:26 Changed 6 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/16583 to 20c767c6a446b441b1551a03a10fb09e1e4bb711
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:27 Changed 6 years ago by jpflori

  • Commit 20c767c6a446b441b1551a03a10fb09e1e4bb711 deleted

Sorry to dig into "old" stuff, but I was suggested to use pxd files rather than pxi files and see the changes here went the other way around... Any strong reason to have done that?

comment:28 Changed 6 years ago by vbraun

Generally speaking, Cython got better at what can be cimported over time (pxd). So there is less reason to rely on textual inclusion (pxi). If in doubt (and if it works ;-) then use pxd.

comment:29 Changed 6 years ago by jpflori

Ok, then I'll take the chance to open a ticket to migrate all of flint stuff to pxd files.

comment:30 Changed 6 years ago by jdemeyer

See #16428.

Note: See TracTickets for help on using tickets.