sage
sage copied to clipboard
sagemath
Wrap ntl_mat_ZZ_p
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
Branch pushed to git repo; I updated commit sha1. New commits:
747c0b4 | actually implement sage rep of ntl |
cc-ing a few people who might be interested and/or have the technical expertises to review this.
-
What is the point of having a Python wrapper that is not a Sage matrix class? This is something I don't understand.
-
Why all the inline methods in
ntlwrap.cppare 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:
-
for i from 0 <= i < _nrows:is an old syntax. You would better userange. -
The copyright header
+#*****************************************************************************
+# Copyright (C) 2005 William Stein <[email protected]>
should contain the author name with the appropriate year.
- This comment should be removed
+##############################################################################
+#
+# ntl_mat_ZZ_p: Matrices over ZZ_p via NTL
+#
+# AUTHORS:
+# - Martin Albrecht <[email protected]>
+# 2006-01: initial version of mat_GF2E (based on code by William Stein)
+# - Alex J. Best <[email protected]>
+# 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.
Replying to @videlec:
- 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 withMatrix_integer_densethat 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_densethat usesimlorlinbox.
Replying to @videlec: Thanks for the comments! I'm pleased to hear some of this can be made neater/more idiomatic.
- 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.
- Why all the inline methods in
ntlwrap.cppare 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:
for i from 0 <= i < _nrows:is an old syntax. You would better userange.The copyright header
+#***************************************************************************** +# Copyright (C) 2005 William Stein <[email protected]>should contain the author name with the appropriate year.
- This comment should be removed
+############################################################################## +# +# ntl_mat_ZZ_p: Matrices over ZZ_p via NTL +# +# AUTHORS: +# - Martin Albrecht <[email protected]> +# 2006-01: initial version of mat_GF2E (based on code by William Stein) +# - Alex J. Best <[email protected]> +# 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!
BTW, what is in NTL that you can not find in the current Sage matrix class?
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.
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.
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_pcould be done without Python wrapper. At #24544 you can have a look atsage/libs/linbox/conversion.pxdthat perform conversionslinbox C++ types <-> Sage typeswithout any intermediate Python wrapper. It saves a lot of code that boils down tocdef 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.
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!
I have tried to rebase.
New commits:
a7b785b | wrap ntl_mat_ZZ_p functionality |
d8cfece | actually implement sage rep of ntl |
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.
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.
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/.
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.
Branch pushed to git repo; I updated commit sha1. New commits:
4fe3cf2 | Merge branch 'public/ticket/25365' in 9.2.b2 |
Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
Setting a new milestone for this ticket based on a cursory review.