Opened 4 years ago

Closed 4 years ago

#21321 closed enhancement (fixed)

Cleanup in sparse modules

Reported by: Bouillaguet Owned by:
Priority: major Milestone: sage-7.4
Component: linear algebra Keywords: sd75
Cc: cpernet Merged in:
Authors: Charles Bouillaguet Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: ce4de73 (Commits) Commit: ce4de7305ff7bf9ddf91fb223f2884b19ce475b9
Dependencies: Stopgaps:

Description (last modified by Bouillaguet)

This ticket removes all the .pxi files in sage/modules, and replaces them by proper .pxd/pyx files.

It adds no new functionnality, but seems to be required to have a C++ binding to linbox.

Change History (23)

comment:1 Changed 4 years ago by Bouillaguet

  • Branch set to u/Bouillaguet/module_pxi_must_die

comment:2 Changed 4 years ago by cpernet

  • Cc cpernet added
  • Commit set to 0d50a4e7953836d11d1baea347087b3a81bae167
  • Keywords sd75 added

New commits:

​0d50a4eremoves .pxi files in sage/modules, replace by .pyx/pxd

comment:3 Changed 4 years ago by git

  • Commit changed from 0d50a4e7953836d11d1baea347087b3a81bae167 to a32caee5c84af293b597434ef5c4449e722ca28e

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

​1126d8aforgot module_list.py
​fd3ee8cconvert modules/vector_rational_sparse.pxi
​b6bc054Merge commit 'fd3ee8c9a45b75c5384546166496622d6a860f15' into develop
​e5c8348fix vector_rational_sparse
​a31fc69convert module/vector_integer_sparse.pxi
​855a549Merge commit 'a31fc697619288b58799a3edb80d41bf070c3d50' into develop
​ef6eecefixed vector_integer_sparse
​f0c88dbconvert modules/vector_modn_sparse.pxi
​2d7ee59Merge branch 'modules_pxi_must_die_hard' into develop
​a32caeefixed vector_modn_sparse

comment:4 Changed 4 years ago by Bouillaguet

  • Authors changed from Bouillaguet to Charles Bouillaguet

comment:5 Changed 4 years ago by Bouillaguet

  • Description modified (diff)
  • Priority changed from minor to major
  • Status changed from new to needs_review

comment:6 Changed 4 years ago by git

  • Commit changed from a32caee5c84af293b597434ef5c4449e722ca28e to 206e794bd92d294d917d4b503a654e13776b5b91

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

​206e794Fixed a remaining reference to vector_modn_sparse.pxi

comment:7 Changed 4 years ago by jdemeyer

I might review this.

comment:8 Changed 4 years ago by jdemeyer

One thing: you have a very complicated git history here. Can you squash it to just 1 commit?

comment:9 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

cysignals stuff should only be in .pyx files.

comment:10 Changed 4 years ago by jdemeyer

More generally, .pxd files should only cimport/include what they really need.

comment:11 follow-up: Changed 4 years ago by jdemeyer

Did you check for conflicts with #17635?

comment:12 Changed 4 years ago by git

  • Commit changed from 206e794bd92d294d917d4b503a654e13776b5b91 to 2036d22686f91b6d6155265a7615be2f947f797d

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

​305adf6sqome more typos
​72730d6Updated SageMath version to 7.4.beta1
​2036d22Convert .pxi files in sage/modules/ into proper .pxd/.pyx files

comment:13 follow-up: Changed 4 years ago by Bouillaguet

  • Status changed from needs_work to needs_review

Thanks for your time, Jeroen. I fixed the extra imports in .pxd files, and sqashed all that into a single commit. Up for review again.

comment:14 in reply to: ↑ 13 Changed 4 years ago by jdemeyer

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

Replying to Bouillaguet:

and sqashed all that into a single commit.

I think you did something wrong, since I see two unrelated commits. In any case, you should rebase on top of sage-7.4.beta2 now.

comment:15 in reply to: ↑ 11 Changed 4 years ago by cpernet

Replying to jdemeyer:

Did you check for conflicts with #17635?

This ticket merges cleanly with 17635 and the testsuite passes on my box.

comment:16 Changed 4 years ago by jdemeyer

Since you're moving files anyway, could you move src/sage/modules/binary_search to src/sage/data_structures/binary_search. That code has nothing to do with modules.

comment:17 Changed 4 years ago by git

  • Commit changed from 2036d22686f91b6d6155265a7615be2f947f797d to b7066138d2cf0730d4eda3af2c1ad315d1dc9e9b

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

​b706613Convert .pxi files in sage/modules/ into proper .pxd/.pyx files

comment:18 Changed 4 years ago by git

  • Commit changed from b7066138d2cf0730d4eda3af2c1ad315d1dc9e9b to ce4de7305ff7bf9ddf91fb223f2884b19ce475b9

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

​ce4de73moved sage.modules.binary_search to sage.data_structures.binary_search

comment:19 Changed 4 years ago by Bouillaguet

  • Status changed from needs_work to needs_review

Jeroen, the ticket is ready for review (again).

comment:20 Changed 4 years ago by jdemeyer

Did you run full doctests with this latest patch?

comment:21 Changed 4 years ago by Bouillaguet

No. I'm counting on the patchbot...

comment:22 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to positive_review

Tests are good.

comment:23 Changed 4 years ago by vbraun

  • Branch changed from u/Bouillaguet/module_pxi_must_die to ce4de7305ff7bf9ddf91fb223f2884b19ce475b9
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.