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 in sparse modules #21321

Closed
sagetrac-Bouillaguet mannequin opened this issue Aug 23, 2016 · 31 comments
Closed

Cleanup in sparse modules #21321

sagetrac-Bouillaguet mannequin opened this issue Aug 23, 2016 · 31 comments

Comments

@sagetrac-Bouillaguet
Copy link
Mannequin

sagetrac-Bouillaguet mannequin commented Aug 23, 2016

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.

CC: @ClementPernet

Component: linear algebra

Keywords: sd75

Author: Charles Bouillaguet

Branch/Commit: ce4de73

Reviewer: Jeroen Demeyer

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

@sagetrac-Bouillaguet sagetrac-Bouillaguet mannequin added this to the sage-7.4 milestone Aug 23, 2016
@sagetrac-Bouillaguet
Copy link
Mannequin Author

sagetrac-Bouillaguet mannequin commented Aug 23, 2016

Branch: u/Bouillaguet/module_pxi_must_die

@ClementPernet
Copy link
Contributor

New commits:

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

@ClementPernet
Copy link
Contributor

Commit: 0d50a4e

@ClementPernet
Copy link
Contributor

Changed keywords from none to sd75

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 23, 2016

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 23, 2016

Changed commit from 0d50a4e to a32caee

@sagetrac-Bouillaguet
Copy link
Mannequin Author

sagetrac-Bouillaguet mannequin commented Aug 23, 2016

Changed author from Bouillaguet to Charles Bouillaguet

@sagetrac-Bouillaguet

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 24, 2016

Changed commit from a32caee to 206e794

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 24, 2016

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

206e794Fixed a remaining reference to vector_modn_sparse.pxi

@jdemeyer
Copy link

comment:7

I might review this.

@jdemeyer
Copy link

comment:8

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

@jdemeyer
Copy link

comment:9

cysignals stuff should only be in .pyx files.

@jdemeyer
Copy link

comment:10

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

@jdemeyer
Copy link

comment:11

Did you check for conflicts with #17635?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 25, 2016

Changed commit from 206e794 to 2036d22

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 25, 2016

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

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

@sagetrac-Bouillaguet
Copy link
Mannequin Author

sagetrac-Bouillaguet mannequin commented Aug 25, 2016

comment:13

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.

@jdemeyer
Copy link

comment:14

Replying to @sagetrac-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.

@jdemeyer
Copy link

Reviewer: Jeroen Demeyer

@ClementPernet
Copy link
Contributor

comment:15

Replying to @jdemeyer:

Did you check for conflicts with #17635?

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

@jdemeyer
Copy link

comment:16

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 26, 2016

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 26, 2016

Changed commit from 2036d22 to b706613

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 27, 2016

Changed commit from b706613 to ce4de73

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 27, 2016

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

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

@sagetrac-Bouillaguet
Copy link
Mannequin Author

sagetrac-Bouillaguet mannequin commented Aug 27, 2016

comment:19

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

@jdemeyer
Copy link

comment:20

Did you run full doctests with this latest patch?

@sagetrac-Bouillaguet
Copy link
Mannequin Author

sagetrac-Bouillaguet mannequin commented Aug 27, 2016

comment:21

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

@jdemeyer
Copy link

comment:22

Tests are good.

@vbraun
Copy link
Member

vbraun commented Aug 29, 2016

Changed branch from u/Bouillaguet/module_pxi_must_die to ce4de73

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

3 participants