Wrap ntl_mat_ZZ_p

[alexjbest]

Wrap some of the functionality for matrices over Z/pZ available in ntl

CC: @edgarcosta @videlec @jdemeyer @simon-king-jena

Component: c_lib

Keywords: libs, sd87

Author: Alex J. Best

Branch/Commit: public/ticket/25365 @ 4fe3cf2

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

[alexjbest]

Branch: u/alexjbest/wrap-ntl-mat-ZZ-p

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

747c0b4

actually implement sage rep of ntl

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Commit: 747c0b4

[tscrim]

comment:4

cc-ing a few people who might be interested and/or have the technical expertises to review this.

[videlec]

comment:5

1. What is the point of having a Python wrapper that is not a Sage matrix class? This is something I don't understand.

2. Why all the inline methods in ntlwrap.cpp are needed at all? Cython has very good support for C++ now. The ntl wrappers in Sage are not up to date and would better be cleaned up.

And minor remarks:

3. for i from 0 <= i < _nrows: is an old syntax. You would better use range.

4. The copyright header

+#*****************************************************************************
+#       Copyright (C) 2005 William Stein <wstein@gmail.com>

should contain the author name with the appropriate year.

5. This comment should be removed

+##############################################################################
+#
+# ntl_mat_ZZ_p: Matrices over ZZ_p via NTL
+#
+# AUTHORS:
+#  - Martin Albrecht <malb@informatik.uni-bremen.de>
+#    2006-01: initial version of mat_GF2E (based on code by William Stein)
+#  - Alex J. Best <alex.j.best@gmail.com>
+#    2018-02: transplanted to mat_ZZ_p
+#
+##############################################################################

You need to convert it in a proper module documentation and add the relevant information in the AUTHOR section. You can find relevant information in the Sage precisely stated in the Sage Developer’s Guide.

[videlec]

comment:7

Replying to @videlec:

1. What is the point of having a Python wrapper that is not a Sage matrix class? This is something I don't understand.

More precisely, you need to choose between:

• Write a complete matrix class whose "backend" would be a NTL c++ class. You would have to inherit from sage.matrix.matrix_dense.Matrix_dense. A good example is provided with Matrix_integer_dense that wraps FLINT.

• Add some options to the methods of the current matrix classes to call NTL when appropriate. To that purpose, you have to convert the datastructure when needed. Some examples are also provided with Matrix_integer_dense that uses iml or linbox.

[alexjbest]

comment:8

Replying to @videlec: Thanks for the comments! I'm pleased to hear some of this can be made neater/more idiomatic.

1. What is the point of having a Python wrapper that is not a Sage matrix class? This is something I don't understand.

I appreciate that this is a little weird, the point is #25366 which is used in #20264, of course in an ideal world I would have implemented a lot more in this ticket. But I thought I would do this as a first step so that those tickets could be added and maybe later see what ntl has to offer sage matrices over Z/nZ more generally.

2. Why all the inline methods in ntlwrap.cpp are needed at all? Cython has very good support for C++ now. The ntl wrappers in Sage are not up to date and would better be cleaned up.

That's great to hear, I'm not particularly up to date on cython, so I just took existing ntl wrappers and modified them until everything worked. Is it sufficient to just delete a lot of lines in ntlwrap now? I can of course go through and do this (for other modules also).

And minor remarks:

3. for i from 0 <= i < _nrows: is an old syntax. You would better use range.

4. The copyright header

+#*****************************************************************************
+#       Copyright (C) 2005 William Stein <wstein@gmail.com>

should contain the author name with the appropriate year.

5. This comment should be removed

+##############################################################################
+#
+# ntl_mat_ZZ_p: Matrices over ZZ_p via NTL
+#
+# AUTHORS:
+#  - Martin Albrecht <malb@informatik.uni-bremen.de>
+#    2006-01: initial version of mat_GF2E (based on code by William Stein)
+#  - Alex J. Best <alex.j.best@gmail.com>
+#    2018-02: transplanted to mat_ZZ_p
+#
+##############################################################################

You need to convert it in a proper module documentation and add the relevant information in the AUTHOR section. You can find relevant information in the Sage precisely stated in the Sage Developer’s Guide.

Yeah most of this was just me copying mat_GF2E so I'll try and fix them both (all?), thanks!

[videlec]

comment:9

BTW, what is in NTL that you can not find in the current Sage matrix class?

[alexjbest]

comment:10

Replying to @videlec:

BTW, what is in NTL that you can not find in the current Sage matrix class?

Its #25366 that is needed this is a function that has mat_Zz_p's for input and output, which as far as i can tell sage does not currently understand at all. so in reality half of what I'm proposing here is not needed/used right now. But it seemed even less useful to just add the couple of accessors we need.

[videlec]

comment:11

Replying to @alexjbest:

Replying to @videlec:

BTW, what is in NTL that you can not find in the current Sage matrix class?

Its #25366 that is needed this is a function that has mat_Zz_p's for input and output, which as far as i can tell sage does not currently understand at all. so in reality half of what I'm proposing here is not needed/used right now. But it seemed even less useful to just add the couple of accessors we need.

It is not clear to me why you need a Python wrapper to call a C++ NTL function. Converting a Sage matrix into a NTL mat_ZZ_p could be done without Python wrapper. At #24544 you can have a look at sage/libs/linbox/conversion.pxd that perform conversions linbox C++ types <-> Sage types without any intermediate Python wrapper. It saves a lot of code that boils down to

cdef class my_wrapping_class:
    def my_method(self):
        return self.wrapped_object.my_method()

If you want to call NTL functions, just provide these conversions.

The wrapper that is provided in the branch would be useful if it provided an alternative implementation of matrices (see also [comment:5] and [comment:7]). Matrices do accept an implementation keyword and one can imagine an implementation using NTL

sage: MatrixSpace(ZZ, 2, implementation='gap')
Full MatrixSpace of 2 by 2 dense matrices over Integer Ring (using Matrix_gap)

But these two purposes are mostly disjoint and I am inclined to think that you are interested in the first situation.

[alexjbest]

comment:12

Replying to @videlec:

Replying to @alexjbest:

Replying to @videlec:

BTW, what is in NTL that you can not find in the current Sage matrix class?

Its #25366 that is needed this is a function that has mat_Zz_p's for input and output, which as far as i can tell sage does not currently understand at all. so in reality half of what I'm proposing here is not needed/used right now. But it seemed even less useful to just add the couple of accessors we need.

It is not clear to me why you need a Python wrapper to call a C++ NTL function. Converting a Sage matrix into a NTL mat_ZZ_p could be done without Python wrapper. At #24544 you can have a look at sage/libs/linbox/conversion.pxd that perform conversions linbox C++ types <-> Sage types without any intermediate Python wrapper. It saves a lot of code that boils down to

cdef class my_wrapping_class:
    def my_method(self):
        return self.wrapped_object.my_method()

If you want to call NTL functions, just provide these conversions.

The wrapper that is provided in the branch would be useful if it provided an alternative implementation of matrices (see also [comment:5] and [comment:7]). Matrices do accept an implementation keyword and one can imagine an implementation using NTL

sage: MatrixSpace(ZZ, 2, implementation='gap')
Full MatrixSpace of 2 by 2 dense matrices over Integer Ring (using Matrix_gap)

But these two purposes are mostly disjoint and I am inclined to think that you are interested in the first situation.

Yes thats a fair assesment, I suppose I thought it would be most helpful to lay a little groundwork for the latter purpose while doing the former. I would like to look into the alternative approach you suggest when I have some time as it does sound better, but that probably wont happen immediately.

[alexjbest]

comment:13

Replying to @videlec:

If you want to call NTL functions, just provide these conversions.

Ok so over at #25366 I have tried to do as you suggest without adding any new classes, it was a fun exercise in permuting code until everything works... If you wouldn't mind checking it out and seeing how it compares to what you had in mind (the code at least, I'll fix all the style, the fact it says linbox still etc. once it seems the code is acceptable) that would be really helpful!

[videlec]

comment:14

Indeed, the code looks good.

[videlec]

comment:15

update milestone 8.3 -> 8.4

[fchapoton]

Changed commit from 747c0b4 to d8cfece

[fchapoton]

comment:16

I have tried to rebase.

---

New commits:

a7b785b	wrap ntl_mat_ZZ_p functionality
d8cfece	actually implement sage rep of ntl

[fchapoton]

Changed branch from u/alexjbest/wrap-ntl-mat-ZZ-p to public/ticket/25365

[videlec]

comment:17

Instead of specifying the dependencies in the extension as

    Extension('sage.libs.ntl.ntl_mat_ZZ_p',
              sources = ["sage/libs/ntl/ntl_mat_ZZ_p.pyx"],
              libraries = ["ntl", "gmp", "m"],
              language='c++'),

it is prefered to put them in the .pxd file via distutils directive (to be put on top of the file)

# distutils: libraries = ntl gmp m

This way, any Cython file that includes the given pxd will be compiled with the appropriate linking.

The same applies to language and you could have a look at the various pxd files in sage/libs.

[videlec]

comment:18

Is there a reason why you add the prefix mat_ZZ_p_ to all function names? Cython perfectly supports function overloading (in C++). Since it is done that way in all other pxd I guess it makes sense. But it would be good to simplify it and stick to the name in the library.

[videlec]

comment:19

Don't put the declaration and implementation of ntl_mat_ZZ_p inside sage/libs. The repo sage/libs is intended for library declarations. All code related to matrices should go in sage/matrix/.

[videlec]

comment:20

And many remarks from 14 months ago have not been adressed.

[embray]

comment:21

Ticket retargeted after milestone closed

[mkoeppe]

comment:22

Batch modifying tickets that will likely not be ready for 9.1, based on a review of the ticket title, branch/review status, and last modification date.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

4fe3cf2

Merge branch 'public/ticket/25365' in 9.2.b2

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from d8cfece to 4fe3cf2

[mkoeppe]

comment:25

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

[mkoeppe]

comment:26

Setting a new milestone for this ticket based on a cursory review.