Proper initialisation for the optional MeatAxe library

[SimonKing]

Modules using the optional MeatAxe package need the following initialisation:

• MeatAxe executables rely on an environment variable declaring where to find multiplication tables. Of course, this needs to be done only once.
• Each module that links against the MeatAxe library, which is a static library, has to define a certain C variable, which serves the same purpose as the aforementioned environment variable.

I suggest to add here a function that does the initialisation and must be called be each module using MeatAxe functions. For that purpose, a new module sage.libs.meataxe is created.

[SimonKing]

New commits:

​9e0f5d4	Fix reading mtx-matrix from non-existent file
​778df7d	Help using MeatAxe executables in Sage
​02fa8a2	Make pickling machine independent
​e861781	Add c(p)def functions to get/set horizontal matrix slices

[git]

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

​4f3d56c

Fix index check in get_slice

[vdelecroix]

I guess you forgot some # optional in the doctests

**********************************************************************
File "src/sage/matrix/matrix_gfpn_dense.pyx", line 657, in sage.matrix.matrix_gfpn_dense.Matrix_gfpn_dense.get_slice
Failed example:
    from sage.matrix.matrix_gfpn_dense import Matrix_gfpn_dense as MTX
Exception raised:
    Traceback (most recent call last):
    ...
    ImportError: No module named matrix_gfpn_dense
**********************************************************************

[git]

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

​a4dded9

Make new tests optional

[SimonKing]

Replying to vdelecroix:

I guess you forgot some # optional in the doctests

Done!

[SimonKing]

The patchbot reports a timout. Is that reproducible?

Also, according to the coverage plugin, it seems that one function lacks testing.

And I found that the function FfTrueRowSize is uselessly called in a couple of places: It always holds true that FfCurrentRowSizeIo == FfTrueRowSize(FfNoc), where FfCurrentRowSizeIo and FfNoc are both some global variables. Nevertheless, there are calls to FfTrueRowSize(FfNoc) in various places. When profiling my cohomology computations, a measurable amount of time was spent on FfTrueRowSize; therefore I want to create another patch.

[git]

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

​e17e4df	Document and test _rowlist_()
​291f4dc	Some minor tweaks for performance

[vdelecroix]

There are strange doctest errors on arando. Could you have a look?

[git]

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

​03ce5c0

Fix bad INPUT::/OUTPUT:: blocks

[SimonKing]

I have pushed another commit, since I made a typo in doc formatting.

Replying to vdelecroix:

There are strange doctest errors on arando. Could you have a look?

Maybe it isn't strange after all. It is about a random test. The values I provide in the test are what I get when I start a fresh Sage session, but probably I forgot that the random seed in doctests is different.

So, easy to fix...

[git]

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

​5ed2269

Fix one doctest

[SimonKing]

Fixed!

[SimonKing]

Admittedly, the bugs fixed here only concern users of the optional meataxe package. However, I'd appreciate a review. At least the patchbot seems happy about it.

[jdemeyer]

For Python 3 compatibility, you should use bytes instead of str for pickling.

This means changing the PyString_... functions to PyBytes_... and this:

-def mtx_unpickle(f, int nr, int nc, str Data, bint m):
+def mtx_unpickle(f, int nr, int nc, bytes Data, bint m):

[jdemeyer]

Replying to SimonKing:

• MeatAxe executable rely on an environment variable declaring where to find multiplication tables.

Are you sure? I thought this was handled by the MtxLibDir array?

[jdemeyer]

This is deprecated syntax:

for i from 0<=i<self.Data.Nor:

Cython understands range().

[jdemeyer]

You're not checking the result of this call:

d = <char*>sig_malloc(pickle_size)

you can use instead

d = check_malloc(pickle_size)

[jdemeyer]

Wrong ticket:

deprecation(12345, "Reading this pickle may be machine dependent")

[jdemeyer]

This function is not used nor tested anywhere:

cdef Matrix_t *rawMatrix(int Field, list entries) except NULL

[jdemeyer]

Replace

 cdef extern from "Python.h":
     object PyString_FromStringAndSize(char *s, Py_ssize_t len)
     char* PyString_AsString(object string)

by

from cpython.bytes cimport PyBytes_FromStringAndSize, PyBytes_AsString

[SimonKing]

Replying to jdemeyer:

Replying to SimonKing:

• MeatAxe executable rely on an environment variable declaring where to find multiplication tables.

Are you sure? I thought this was handled by the MtxLibDir array?

Yes, I am sure. Setting the MtxLibDir variable in your programs works if your program calls mtx library functions. It will not work when you call a meataxe executable that does *not* set the MtxLibDir variable. That's what the envirionment variable is for.

[jdemeyer]

Replying to SimonKing:

It will not work when you call a meataxe executable that does *not* set the MtxLibDir variable.

And which part of src/sage/matrix/matrix_gfpn_dense.pyx calls a meataxe executable?

You could set them in sage-env where many similar variables are handled.

[SimonKing]

Replying to jdemeyer:

Replying to SimonKing:

It will not work when you call a meataxe executable that does *not* set the MtxLibDir variable.

And which part of src/sage/matrix/matrix_gfpn_dense.pyx calls a meataxe executable?

My modular group cohomology package does. It was made dysfunctional by changes in Sage, and my long-standing plan is to turn it into a new style package.

[jdemeyer]

Replying to SimonKing:

My modular group cohomology package does.

Then I think you should either deal with those environment variables in your package or in sage-env. But not in src/sage/matrix/matrix_gfpn_dense.pyx which has absolutely nothing to do with running meataxe executables.

[jdemeyer]

There are also some very dubious uses of sig_on() in src/sage/matrix/matrix_gfpn_dense.pyx, but let's keep that for a different ticket.

[SimonKing]

It seems that I forgot to import a couple of things: sig_malloc, sig_free.

Currently I am using sig_malloc and sig_free, but check_realloc. I wonder whether I use these functions correctly. Reading the code in cysignals: Do I see correctly that sig_* functions would not raise an error (so that I have to test whether they return NULL, in which case I have to raise my own error), whereas check_* would raise an error?

[jdemeyer]

Replying to SimonKing:

Reading the code in cysignals: Do I see correctly that sig_* functions would not raise an error (so that I have to test whether they return NULL, in which case I have to raise my own error), whereas check_* would raise an error?

Yes: sig_malloc would just return NULL when an allocation failed while check_allocarray would raise MemoryError.

[git]

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

​d4dc794	Merge branch 'develop' into t/21437/fix-mtx-matrices
​413e4a9	Remove deprecated Cython syntax, use check_malloc
​d943f93	Remove rawMatrix function
​c929fcc	Remove unused MTX* environment variables

[SimonKing]

I think I have now addressed all your remarks, so, I put it back to "needs review".

One question, though. While building the code, I see one warning:

[sagelib-8.0.rc1] /home/king/Sage/git/sage/src/build/cythonized/sage/matrix/matrix_gfpn_dense.c: In function 'initmatrix_gfpn_dense':
[sagelib-8.0.rc1] /home/king/Sage/git/sage/src/build/cythonized/sage/matrix/matrix_gfpn_dense.c:19604:22: warning: passing argument 1 of 'MtxSetErrorHandler' from incompatible pointer type [-Wincompatible-pointer-types]
[sagelib-8.0.rc1]    MtxSetErrorHandler(__pyx_f_4sage_6matrix_17matrix_gfpn_dense_ErrorHandler);
[sagelib-8.0.rc1]                       ^
[sagelib-8.0.rc1] In file included from /home/king/Sage/git/sage/src/build/cythonized/sage/matrix/matrix_gfpn_dense.c:479:0:
[sagelib-8.0.rc1] /home/king/Sage/git/sage/local/include/meataxe.h:371:20: note: expected 'void (*)(const MtxErrorRecord_t *) {aka void (*)(const struct <anonymous> *)}' but argument is of type 'void (*)(MtxErrorRecord_t *) {aka void (*)(struct <anonymous> *)}'
[sagelib-8.0.rc1]  MtxErrorHandler_t *MtxSetErrorHandler(MtxErrorHandler_t *h);
[sagelib-8.0.rc1]                     ^

How could I fix that?

[SimonKing]

Replying to jdemeyer:

There are also some very dubious uses of sig_on() in src/sage/matrix/matrix_gfpn_dense.pyx, but let's keep that for a different ticket.

Would you insist on a different ticket, or could I try to fix it here?

[SimonKing]

I guess in some places I also have to replace cdef str by cdef bytes...

[git]

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

​b70bc5f

Change some str into bytes

[SimonKing]

Replying to SimonKing:

I guess in some places I also have to replace cdef str by cdef bytes...

Done. But I think I should also have a look at the sig_on() thingy...

[SimonKing]

Question: By "dubious use of sig_on", do you mean stuff like the following?

                sig_on()
                if nc%MPB:
                    for i in range(nr):
                        y = <unsigned char*>x
                        for j in range(FfCurrentRowSizeIo-1):
                            y[j] = randint()%O
                        y[FfCurrentRowSizeIo-1] = randint()%(fl**(nc%MPB))
                        FfStepPtr(&(x))
                else:
                    for i in range(nr):
                        y = <unsigned char*>x
                        for j in range(FfCurrentRowSizeIo):
                            y[j] = randint()%O
                        FfStepPtr(&(x))
                sig_off()

Somewhere I was reading that sig_on/sig_off should be used if there are potentially long c commands, whereas in the code above (only very short commands are executed in tight loops) one should use sig_check.

Did I describe the intended use of sig_* correctly?

[git]

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

​04e7986

Improve use of sig_on/off/check

[SimonKing]

Replying to SimonKing:

Did I describe the intended use of sig_* correctly?

I hope that the last commit did improve the situation.

[SimonKing]

I hate git.

My plan was to merge this ticket with #23352 (from my point of view it doesn't matter what is merged into what).

But there is a merge conflict. Although #23352 only has a very small diff, git apparently reports conflicts all over the place. WTF? In #23352, we just have

src/sage/matrix/matrix_gfpn_dense.pyx

@@ -703 +703 @@
             [2*z^2 + 2*z + 2               0               0   2*z^2 + z + 2               0         2*z + 1]
             [              0       2*z^2 + z               0               1               0   2*z^2 + z + 1]
 
+        The following tests against a bug that was fixed in :trac:`23352`::
+
+            sage: MS = MatrixSpace(GF(9,'x'),1,5)
+            sage: MS.random_element()     # optional: meataxe
+            [x + 1     x     2 x + 2 x + 2]
+
         """
         self.check_mutability()
         cdef int fl = self.Data.Field
@@ -741 +747 @@
                         y = <unsigned char*>x
                         for j from 0 <= j < FfCurrentRowSizeIo-1:
                             y[j] = randint()%O
-                        y[FfCurrentRowSizeIo-1] = randint()%(fl**(nc%MPB))
+                        for j from nc-(nc%MPB) <= j < nc:
+                            FfInsert(x, j, FfFromInt( (randint()%fl) ))
                         FfStepPtr(&(x))
                 else:
                     for i from 0 <= i < nr:

By definition, we don't have conflicts in parts of the file that are untouched by the above diff!

Really, I hate git.

[SimonKing]

Replying to jdemeyer:

Replying to SimonKing:

It will not work when you call a meataxe executable that does *not* set the MtxLibDir variable.

And which part of src/sage/matrix/matrix_gfpn_dense.pyx calls a meataxe executable?

You could set them in sage-env where many similar variables are handled.

Working on #18514, I found that the situation is confusing for me.

My plan was:

• With the current code from here and #23352, sage.matrix.matrix_gfpn_dense overrides the MtxLibDir variable that is declared in sage.libs.meataxe, but it doesn't touch the environment variables.
• The cohomology code cimports MtxLibDir from sage.matrix.matrix_gfpn_dense and reads its content. It is then also written into an environment variable, so that the MTX-executables can find the multiplication tables.

The reality is:

• When the cohomology code cimports MtxLibDir, its content is a certain location in /usr/local/, which is where the multiplication tables can NOT be found. Thus, any computation crashes.

If I recall correctly, the reason for the bad behaviour is the fact that the meataxe library is a dynamic, not a static, library. Hence, any Cython module and any C program using meataxe has to define MtxLibDir explicitly: Cimporting it won't suffice.

[SimonKing]

The following would work:

• Define MtxLibDir in sage.matrix.matrix_gfpn_dense.
• Use a copy, say, _MtxLibDr
• As it turns out, when cimporting _MtxLibDir, one gets the correct value. Hence, one uses _MtxLibDir to override MtxLibDir in any module that uses the meataxe library.

Do you agree to add _MtxLibDir in this ticket, although I will only need it in #18514?

[git]

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

​6a65600

Use a fixed copy of MtxLibDir

[SimonKing]

Replying to SimonKing:

Do you agree to add _MtxLibDir in this ticket, although I will only need it in #18514?

... as in the latest commit.

[jdemeyer]

Replying to SimonKing:

Question: By "dubious use of sig_on", do you mean stuff like the following?

Not quite, that example is acceptable, although you certainly could sig_check() there. I meant cases where Python code is inside sig_on() blocks, which shouldn't be done.

[jdemeyer]

Concerning MtxLibDir: I think that part of the problem comes from insufficient separation between (1) the MeatAxe library and (2) the implementation of matrices using MeatAxe.

Things like dealing with the MtxLibDir and also the error handler really have nothing to do with matrices and should be moved to src/sage/libs/meataxe.pyx. If you set MtxLibDir in src/sage/libs/meataxe.pyx and you import sage.libs.meataxe, then that should work too and it's a much cleaner solution.

[SimonKing]

Replying to jdemeyer:

Concerning MtxLibDir: I think that part of the problem comes from insufficient separation between (1) the MeatAxe library and (2) the implementation of matrices using MeatAxe.

Things like dealing with the MtxLibDir and also the error handler really have nothing to do with matrices and should be moved to src/sage/libs/meataxe.pyx. If you set MtxLibDir in src/sage/libs/meataxe.pyx and you import sage.libs.meataxe, then that should work too and it's a much cleaner solution.

OK, but it means that I need to add a new module (currently, there is only the pxd file src/sage/libs/meataxe.pxd), and it also means that I need to import, right? "cimport" wouldn't suffice?

[SimonKing]

Replying to jdemeyer:

Replying to SimonKing:

Question: By "dubious use of sig_on", do you mean stuff like the following?

Not quite, that example is acceptable, although you certainly could sig_check() there. I meant cases where Python code is inside sig_on() blocks, which shouldn't be done.

I think I have removed all such cases. sig_on() now only appears three times in the code, namely

        sig_on()
        OUT.Data = MatDup(self.Data)
        MatMul(OUT.Data,right.Data)
        sig_off()

        sig_on()
        MatMulStrassen(OUT.Data, self.Data, right.Data)
        sig_off()

        sig_on()
        try:
            OUT.Data = MatInverse(self.Data)
        except ZeroDivisionError:
            sig_off()
            raise
        sig_off()

It doesn't involve python code, right?

[jdemeyer]

The error handler involves Python code. So to be extra-safe, it would be better to surround the call to PyErr_SetObject with sig_block()/sig_unblock().

PS: 12437 is still not the correct Trac ticket.

[jdemeyer]

Replying to SimonKing:

OK, but it means that I need to add a new module (currently, there is only the pxd file src/sage/libs/meataxe.pxd)

Yes, that is what I meant.

and it also means that I need to import, right? "cimport" wouldn't suffice?

Yes, I think so. You need import sage.libs.meataxe (there is nothing you need to import from that, but you need to ensure that the module is loaded).

[jdemeyer]

22 has not been addressed

[SimonKing]

Replying to jdemeyer:

Replying to SimonKing:

OK, but it means that I need to add a new module (currently, there is only the pxd file src/sage/libs/meataxe.pxd)

Yes, that is what I meant.

and it also means that I need to import, right? "cimport" wouldn't suffice?

Yes, I think so. You need import sage.libs.meataxe (there is nothing you need to import from that, but you need to ensure that the module is loaded).

No, it doesn't work. Apparently each cython module that is linked against the dynamic meataxe library needs to do the assignment of MtxLibDir and also needs to fix the error handler.

And comment:22 *has* been addressed. There is no PyString in the files.

[git]

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

​67a85ca

Move meataxe definitions from sage.matrix to sage.libs

[jdemeyer]

Sorry, I meant replacing

cdef extern from "Python.h":
    char* PyBytes_AsString(object string)

by

from cpython.bytes cimport PyBytes_AsString

Note also that Cython can auto-convert between bytes and char*, so this function might not be needed at all.

[SimonKing]

Replying to SimonKing:

Replying to jdemeyer:

Replying to SimonKing:

OK, but it means that I need to add a new module (currently, there is only the pxd file src/sage/libs/meataxe.pxd)

Yes, that is what I meant.

and it also means that I need to import, right? "cimport" wouldn't suffice?

Yes, I think so. You need import sage.libs.meataxe (there is nothing you need to import from that, but you need to ensure that the module is loaded).

No, it doesn't work. Apparently each cython module that is linked against the dynamic meataxe library needs to do the assignment of MtxLibDir and also needs to fix the error handler.

Example, with the latest commit:

sage: M = random_matrix(GF(81,'x'),1000)
MTX location /usr/local/mtx/lib
os.c(254):p081.zzz: No such file or directory

which is an unhandled crash. So, both MtxLibDir is not visible for sage.matrix.matrix_gfpn_dense (otherwise meataxe would find the multiplication table) and the error handler is not defined (otherwise it would raise an error rather than crash).

How can this be overcome?

[SimonKing]

Replying to SimonKing:

... So, both MtxLibDir is not visible for sage.matrix.matrix_gfpn_dense (otherwise meataxe would find the multiplication table) and the error handler is not defined (otherwise it would raise an error rather than crash).

How can this be overcome?

By this, I mean: We have modules A,B,C, and A is supposed to do some initialisation to a dynamic library that A, B and C link against. Is it possible to make it so that "import A" in B and C automatically executes code that will do the initialisation in B resp. in C rather than only in A?

Here, we have A=sage.libs.meataxe, B=sage.matrix.matrix_gfpn_dense and C=the cohomology code from #18514

[SimonKing]

A potential solution would be to write sage.libs.meataxe.pxi, and then include the .pxi file rather than cimport.

However, that sounds like an ugly hack.

[jdemeyer]

Replying to SimonKing:

If I recall correctly, the reason for the bad behaviour is the fact that the meataxe library is a dynamic, not a static, library.

You're swapping "static" and "dynamic" but otherwise you are right: the problem with static libraries is that each Cython module gets its own "copy" of the static library.

[jdemeyer]

Looking at the MeatAxe? Makefile, there is no support for building as dynamic library.

[SimonKing]

Replying to jdemeyer:

Looking at the MeatAxe? Makefile, there is no support for building as dynamic library.

Would it be difficult to add support? After all, we are patching meataxe anyway.

[jdemeyer]

Replying to SimonKing:

Would it be difficult to add support? After all, we are patching meataxe anyway.

Instead of "adding support", I would prefer to rewrite the build system to use autotools. Probably not hard, but not fun either.

[jdemeyer]

Replying to SimonKing:

A potential solution would be to write sage.libs.meataxe.pxi, and then include the .pxi file rather than cimport.

Here is a better idea: in meataxe.pxd, define an inline function

cdef inline init_meataxe():
    # set up MtxLibDir and error handler

Then you just need to call init_meataxe() in the module(s) where you want to use MeatAxe.

[SimonKing]

Replying to jdemeyer:

Replying to SimonKing:

A potential solution would be to write sage.libs.meataxe.pxi, and then include the .pxi file rather than cimport.

Here is a better idea: in meataxe.pxd, define an inline function

cdef inline init_meataxe():
    # set up MtxLibDir and error handler

Then you just need to call init_meataxe() in the module(s) where you want to use MeatAxe.

Let's see if it works. I did the same with a function defined in sage.libs.meataxe.pyx (that I added only for that purpose), and it did not work.

[jdemeyer]

Replying to SimonKing:

Let's see if it works. I did the same with a function defined in sage.libs.meataxe.pyx (that I added only for that purpose), and it did not work.

The difference between an inline function and a regular function is crucial here.

[SimonKing]

Replying to SimonKing:

Here is a better idea: in meataxe.pxd, define an inline function

cdef inline init_meataxe():
    # set up MtxLibDir and error handler

Then you just need to call init_meataxe() in the module(s) where you want to use MeatAxe.

Doesn't work. At least not when I do the following:

In sage/libs/meataxe.pxd

cdef inline meataxe_init():
    ## Define an environment variable that enables MeatAxe to find
    ## its multiplication tables.
    from sage.env import DOT_SAGE
    MtxLibDir = PyBytes_AsString(os.path.join(DOT_SAGE,'meataxe'))

In sage/matrix/matrix_gfpn_dense.pyx:

print("MTX location", MtxLibDir)
meataxe_init()
print("New location", MtxLibDir)

Before and after calling meataxe_init() I get the same wrong location.

[SimonKing]

I guess the problem is the import statement in the "cdef inline" function. IIRC, "inline" is only a hint for the compiler, not a strict order. And apparently the compiler decided to not inline it.

[jdemeyer]

Replying to SimonKing:

cdef inline meataxe_init():
    ## Define an environment variable that enables MeatAxe to find
    ## its multiplication tables.
    from sage.env import DOT_SAGE
    MtxLibDir = PyBytes_AsString(os.path.join(DOT_SAGE,'meataxe'))

That's a beginner Python bug :-)

You need global MtxLibDir otherwise MtxLibDir will be treated as local variable.

[jdemeyer]

Replying to SimonKing:

I guess the problem is the import statement in the "cdef inline" function. IIRC, "inline" is only a hint for the compiler, not a strict order. And apparently the compiler decided to not inline it.

No, this is about the Cython interpretation of inline. Whether the C compiler inlines it or not is irrelevant.

[SimonKing]

Replying to jdemeyer:

Replying to SimonKing:

I guess the problem is the import statement in the "cdef inline" function. IIRC, "inline" is only a hint for the compiler, not a strict order. And apparently the compiler decided to not inline it.

No, this is about the Cython interpretation of inline. Whether the C compiler inlines it or not is irrelevant.

Anyway, it doesn't work.

[SimonKing]

Replying to jdemeyer:

Replying to SimonKing:

cdef inline meataxe_init():
    ## Define an environment variable that enables MeatAxe to find
    ## its multiplication tables.
    from sage.env import DOT_SAGE
    MtxLibDir = PyBytes_AsString(os.path.join(DOT_SAGE,'meataxe'))

In the same pxd file:

    cdef extern char MtxLibDir[1024]    # Where to search/create multiplication tables

Is "global" needed, then?

That's a beginner Python bug :-)

You need global MtxLibDir otherwise MtxLibDir will be treated as local variable.

[jdemeyer]

Replying to SimonKing:

Is "global" needed, then?

Yes. The rules for global/local variables in Cython are exactly the same as for Python.

[SimonKing]

The following doesn't work either:

cdef inline meataxe_init():
    ## Define an environment variable that enables MeatAxe to find
    ## its multiplication tables.
    global MtxLibDir
    MtxLibDir = PyBytes_AsString(os.path.join(DOT_SAGE,'meataxe'))
    ## Error handling for MeatAxe, to prevent immediate exit of the program
    MtxSetErrorHandler(ErrorHandler)

Here, I have moved the definition into a pyx file, where I do all the python imports and also the definition of a the dictionary that is used in the ErrorHandler --- am I right that it would be problematic to define the dictionary in a pxd file?

[jdemeyer]

Replying to SimonKing:

Here, I have moved the definition into a pyx file

Don't! This MUST be inline and it MUST be in the .pxd file.

[jdemeyer]

Replying to SimonKing:

am I right that it would be problematic to define the dictionary in a pxd file?

Probably yes. You can move the error handler itself to the .pyx file but the call MtxSetErrorHandler in init_meataxe() in the .pxd file. Something like:

cdef void ErrorHandler(...)

cdef inline meataxe_init():
    ## Define an environment variable that enables MeatAxe to find
    ## its multiplication tables.
    global MtxLibDir
    MtxLibDir = PyBytes_AsString(os.path.join(DOT_SAGE,'meataxe'))
    ## Error handling for MeatAxe, to prevent immediate exit of the program
    MtxSetErrorHandler(ErrorHandler)

[SimonKing]

Replying to jdemeyer:

Replying to SimonKing:

am I right that it would be problematic to define the dictionary in a pxd file?

Probably yes. You can move the error handler itself to the .pyx file but the call MtxSetErrorHandler in init_meataxe() in the .pxd file.

Hooray, that seems to work!

[jdemeyer]

BTW: I would change the name ErrorHandler to something more descriptive, like sage_meataxe_error_handler or something.

[git]

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

​353a8b9

meataxe_init() inline function

[SimonKing]

Am I right in guessing that I shouldn't simplify the git history of this ticket? After all, there are commits that just revert what previous commits have done.

---

New commits:

​353a8b9

meataxe_init() inline function

[git]

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

​08e03c2

Change ErrorHandler into sage_meataxe_error_handler

[jdemeyer]

Replying to SimonKing:

Am I right in guessing that I shouldn't simplify the git history of this ticket?

It depends mainly on whether you think that somebody else is building on top of your branch here. I guess chances are very small , so you could simplify the git history. Just keep in mind #23352.

[jdemeyer]

For Python 3 compatibility, it would be good to add

from __future__ import absolute_import, division

in src/sage/matrix/matrix_gfpn_dense.pyx

Then you should also use floor division if that's what you want, for example here:

cdef size_t pickled_rowsize = len(Data)/nr

There might be other such places too, I have not checked.

[jdemeyer]

Replying to SimonKing:

• There are some auxiliary functions that I would like to use in my group cohomology package (related with slicing).

Since this patch is getting rather big, would it be possible to separate this part? Then we could keep this ticket just to fix things, not adding new functionality.

If you feel up to it, I would actually suggest to split it up even further. For example, the pickling fixes alone deserve a separate ticket.

NOTE: I'm not saying this just out of principle. My experience is that it's a lot easier to get multiple small tickets reviewed than one big ticket.

[jdemeyer]

The deprecation ticket number is still wrong.

[jdemeyer]

About this comment:

###############################################################
## It is needed to do some initialisation. Since meataxe is
## a dynamic library, it is needed to do this initialisation
## in all modules cimporting sage.libs.meataxe. So, you HAVE
## to call the meataxe_init() function when you cimport sage.libs.meataxe!

1. Replace "dynamic" by "static"

2. It's not really correct that meataxe_init() must be called whenever sage.libs.meataxe is cimported. It's more correct to say that meataxe_init() must be called whenever the MeatAxe library is used. This may seem like a silly difference, but it is possible that some module needs access to some MeatAxe type (say, to pass around pointers or for simple conversions) without actually using anything from the MeatAxe static library.

[git]

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

​b6c8114

Make matrix_gfpn_dense python 3 compliant; fix a deprecation

[SimonKing]

If I see that correctly, the following git commits belong to this ticket:

• b6c8114 - (HEAD -> t/21437/fix-mtx-matrices) Make matrix_gfpn_dense python 3 compliant; fix a deprecation (20 seconds ago) <Simon King>
• 08e03c2 - Change ErrorHandler? into sage_meataxe_error_handler (48 minutes ago) <Simon King>
• 353a8b9 - meataxe_init() inline function (49 minutes ago) <Simon King>
• 67a85ca - Move meataxe definitions from sage.matrix to sage.libs (4 hours ago) <Simon King>
• 6a65600 - Use a fixed copy of MtxLibDir? (2 days ago) <Simon King>
• 04e7986 - Improve use of sig_on/off/check (2 days ago) <Simon King>
• b70bc5f - Change some str into bytes (2 days ago) <Simon King>
• c929fcc - Remove unused MTX* environment variables (2 days ago) <Simon King>
• d943f93 - Remove rawMatrix function (2 days ago) <Simon King>
• 413e4a9 - Remove deprecated Cython syntax, use check_malloc (2 days ago) <Simon King>
• 5ed2269 - Fix one doctest (10 months ago) <Simon King>
• 03ce5c0 - Fix bad INPUT::/OUTPUT:: blocks (10 months ago) <Simon King>
• 291f4dc - Some minor tweaks for performance (10 months ago) <Simon King>
• e17e4df - Document and test _rowlist_() (10 months ago) <Simon King>
• a4dded9 - Make new tests optional (10 months ago) <Simon King>
• 4f3d56c - Fix index check in get_slice (10 months ago) <Simon King>
• e861781 - Add c(p)def functions to get/set horizontal matrix slices (10 months ago) <Simon King>
• 02fa8a2 - Make pickling machine independent (10 months ago) <Simon King>
• 778df7d - Help using MeatAxe? executables in Sage (10 months ago) <Simon King>
• 9e0f5d4 - Fix reading mtx-matrix from non-existent file (10 months ago) <Simon King>

On top of that, #23352 adds one more commit:

• 6e33226 - (t/23352/fix_random_matrix_gfpn_dense) Properly fill the last few columns of random meataxe matrices (2 days ago) <Simon King>

OK, quite a lot...

How to split that into smaller chunks? I see the following four topics:

1. Bug fixes, for example:
   • 6e33226 - Properly fill the last few columns of random meataxe matrices
   • 02fa8a2 - Make pickling machine independent
   • 9e0f5d4 - Fix reading mtx-matrix from non-existent file
2. Code style/cleanliness, for example:
   • b6c8114 - Make matrix_gfpn_dense python 3 compliant
   • 04e7986 - Improve use of sig_on/off/check
   • b70bc5f - Change some str into bytes
   • 413e4a9 - Remove deprecated Cython syntax, use check_malloc
   • d943f93 - Remove rawMatrix function
3. Proper MeatAxe initialisation, for example
   • 08e03c2 - Change ErrorHandler?? into sage_meataxe_error_handler
   • 353a8b9 - meataxe_init() inline function
   • 67a85ca - Move meataxe definitions from sage.matrix to sage.libs
   • c929fcc - Remove unused MTX* environment variables
4. Additions, for example
   • e17e4df - Document and test _rowlist_()
   • e861781 - Add c(p)def functions to get/set horizontal matrix slices

Since this ticket's name is about "addition", I suggest to deal with 4. *here*.

Since #23352 is about fixing something, I suggest to deal with 1. *there*, and make it a dependency of everything else.

And then there should be two new tickets for 2. and 3. Probably I will not cherry-pick commits, but will go through the whole diff and manually move stuff to the different topics. Does that sound reasonable to you?

---

New commits:

​b6c8114

Make matrix_gfpn_dense python 3 compliant; fix a deprecation

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

​225afe1	Properly fill the last few columns of random meataxe matrices
​f6722be	Add c(p)def functions to get/set horizontal matrix slices
​6f7caf9	Document and test _rowlist_()
​bd1f3f1	Fix ticket number in deprecation warning
​35b4ee7	Merge branch 't/23352/meataxe_fixes' into t/23399/some_additions_to_matrix_mod_gfpn_dense
​dac9f37	Replace 'for...from' by '...in range()'
​b97839c	Change some str into bytes
​6b6642c	Improve use of sig_check/on/off
​0fb429b	Make matrix_gfpn_dense Python 3 compliant
​1d86d5a	Remove MTX* environment variables, move MeatAxe code to sage.libs

[SimonKing]

From my perspective, the work is done. Thus, together with #23352, #23399 and #23400, it needs review!

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​4960a81	Fix reading mtx-matrix from non-existent file
​4e85fca	Make pickling of matrix_gfpn_dense machine independent
​f9d878a	Add test for reading from non-existing file; add cimports
​bdfb799	Merge branch 't/23411/fix_pickling_of_matrix_gfpn_dense' into t/21437/meataxe_initialisation_merge
​bbd6a61	Properly fill last column of random matrix_gfpn_dense
​6bc9168	Merge branch 't/23352/fix_random_matrix_gfpn_dense' into t/21437/meataxe_initialisation_merge
​23ebebd	Add MeatAxe initialisation function

[SimonKing]

I hope it is ok that this now has three dependencies...

[SimonKing]

#23410 is not a dependency (and merges cleanly). So, let's simplify the list of dependencies.

[git]

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

​e477948	Test meataxe unpickling being machine independent
​01e2c0d	Mark new meataxe test 'optional'
​d174835	Check bounds when unpickling meataxe matrices
​8873330	Mark further meataxe tests optional; fix potential zero division
​7a1ed5a	Meataxe unpickling: Further consistency checks
​1c0dcd5	Merge branch 't/23411/fix_pickling_of_matrix_gfpn_dense' into t/23400/cleanup_of_matrix_gfpn_dense
​71abd44	Speedup meataxe matrix randomisation
​e68140e	Replace sig_on/off by sig_check in meataxe matrix randomisation
​82394bc	Merge branch 't/23352/fix_random_matrix_gfpn_dense' into t/23400/cleanup_of_matrix_gfpn_dense

[SimonKing]

Manual merging of the last few commits of #23352 was needed.

[jdemeyer]

Better use RuntimeError instead of SystemError for unknown error messages.

[SimonKing]

Replying to jdemeyer:

Better use RuntimeError instead of SystemError for unknown error messages.

OK. Also, I found another case where I could be more specific (namely: If a file of a given name doesn't exist, which I guess should result in an !IOError).

[jdemeyer]

Replying to SimonKing:

Replying to jdemeyer:

Better use RuntimeError instead of SystemError for unknown error messages.

OK. Also, I found another case where I could be more specific (namely: If a file of a given name doesn't exist, which I guess should result in an !IOError).

OSError instead of IOError because IOError is just an alias of OSError in Python 3.

[SimonKing]

Replying to jdemeyer:

OK. Also, I found another case where I could be more specific (namely: If a file of a given name doesn't exist, which I guess should result in an !IOError).

OSError instead of IOError because IOError is just an alias of OSError in Python 3.

In that case I should change the current IOError as well. And since #23411 introduces another test with a SystemError that should better be an OSError, I'll merge #23411.

[git]

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

​f274beb	Meataxe unpickling: Further corner cases
​6586e97	Sanitise meataxe matrix initialisation
​77bc58b	Fix indentation in meataxe matrix __init__ doc
​5106ebb	Simplified code path for uninitialised meataxe matrices
​3d94133	Remove codepath for creating uninitialised meataxe matrix
​564104e	Don't use six.string_types, but basestring
​982755f	Workaround: MeatAxe's MatLoad returns NULL on empty filename without setting error
​75ab906	Merge branch 't/23411/fix_pickling_of_matrix_gfpn_dense' into t/21437/meataxe_initialisation
​6cba43e	Improve handling of different meataxe error types

[SimonKing]

Replying to jdemeyer:

Replying to SimonKing:

Replying to jdemeyer:

Better use RuntimeError instead of SystemError for unknown error messages.

OK. Also, I found another case where I could be more specific (namely: If a file of a given name doesn't exist, which I guess should result in an !IOError).

OSError instead of IOError because IOError is just an alias of OSError in Python 3.

Done and done!

[SimonKing]

Replying to SimonKing:

Replying to jdemeyer:

Replying to SimonKing:

Replying to jdemeyer:

Better use RuntimeError instead of SystemError for unknown error messages.

OK. Also, I found another case where I could be more specific (namely: If a file of a given name doesn't exist, which I guess should result in an !IOError).

OSError instead of IOError because IOError is just an alias of OSError in Python 3.

Done and done!

I think I have addressed all objections...

[tscrim]

Jeroen, do you plan on finishing the review on this ticket? You already have been through this code and are more of a Cython expert than I am. However, I can probably finishing reviewing this ticket if you'd prefer.

[jdemeyer]

Viewing the diff of this ticket fails with HTTP error 500. I just sent an email to sagemath-admins for this.

[embray]

For now the best thing to do might be to rebase this on the latest develop branch and then git push -f. SimonKing?: Let me know if you need any help with the git-fu.

[jdemeyer]

I just did that.

---

New commits:

​656dc9c

Merge tag '8.1.beta3' into t/21437/fix-mtx-matrices

[jdemeyer]

This is not going to work on Python 3:

MtxLibDir = PyBytes_AsString(os.path.join(DOT_SAGE, 'meataxe'))

(the reason is that os.path.join(DOT_SAGE, 'meataxe') is unicode, not bytes.

I'm not sure if this should hold back this ticket, there are probably many issues like this in Sage.

[jdemeyer]

Some imports can be cleaned up.

You can remove

cdef extern from "Python.h":
    object PyString_FromStringAndSize(char *s, Py_ssize_t len)
    char* PyString_AsString(object string)

and

####################
#
# auxiliary functions
#
####################
import sys
from libc.string cimport memcpy

I would also remove

from libc.stdlib cimport free

and replace free() by sig_free() in free(old).

[embray]

Let me think about this for a sec. While it's true there are many issues like this in Sage, if we're aware of one now it would be better to address it now rather than make people working on the Python 3 port (i.e. Frédéric) pull their hair out later.

For one, why is it unicode (on Python 2) in the first place?

[jdemeyer]

Replying to embray:

For one, why is it unicode (on Python 2) in the first place?

It's really str which is bytes on Python 2 but unicode (a.k.a. str) on Python 3.

[jdemeyer]

Replying to embray:

Let me think about this for a sec. While it's true there are many issues like this in Sage, if we're aware of one now it would be better to address it now rather than make people working on the Python 3 port (i.e. Frédéric) pull their hair out later.

Right, but we might want to consider implementing/using a general solution to the problem of converting filenames (or strings in general) from unicode to bytes. Something along the lines of ​https://github.com/defeo/cypari2/blob/master/cypari2/string_utils.pyx

[SimonKing]

What should os.path.join(DOT_SAGE, 'meataxe') be replaced with? Shouldn't both Python2 and Python3 just do the right thing, given whatever type DOT_SAGE and 'meataxe' are in the respective Python version?

[SimonKing]

Replying to SimonKing:

What should os.path.join(DOT_SAGE, 'meataxe') be replaced with? Shouldn't both Python2 and Python3 just do the right thing, given whatever type DOT_SAGE and 'meataxe' are in the respective Python version?

Sorry, I just realise that PyBytes_AsString is the probable culprit.

In any case, can you tell me precisely what the line should be replaced with, so that it works both in Python2 and Python3?

[jdemeyer]

Replying to SimonKing:

In any case, can you tell me precisely what the line should be replaced with, so that it works both in Python2 and Python3?

It's not so easy, see 114

[SimonKing]

Replying to jdemeyer:

Replying to SimonKing:

In any case, can you tell me precisely what the line should be replaced with, so that it works both in Python2 and Python3?

It's not so easy, see 114

If I understand correctly, it currently has to stay as it is and needs to be addressed as soon as Sage really moves on.

So, for now, I will change the code only according to your other objections.

[SimonKing]

Replying to SimonKing:

If I understand correctly, it currently has to stay as it is and needs to be addressed as soon as Sage really moves on.

So, for now, I will change the code only according to your other objections.

Done. Why did sage -git trac push not change the commit field but only the branch field? I changed the commit field manually.

[embray]

Replying to jdemeyer:

Replying to embray:

Let me think about this for a sec. While it's true there are many issues like this in Sage, if we're aware of one now it would be better to address it now rather than make people working on the Python 3 port (i.e. Frédéric) pull their hair out later.

Right, but we might want to consider implementing/using a general solution to the problem of converting filenames (or strings in general) from unicode to bytes. Something along the lines of ​https://github.com/defeo/cypari2/blob/master/cypari2/string_utils.pyx

I see. I didn't realize that wasn't already the case. Is there any reason these have to be written in Cython? I don't see functions like these getting much speedup from it. In any case , maybe I can just copy these directly into Sage and sprinkle them around the appropriate places.

[jdemeyer]

Replying to embray:

I don't see functions like these getting much speedup from it.

You do get the speedup from calling those functions (calling a cdef function vs. calling a Python function).

And for cypari2, Cython was an obvious choice since cypari2 is completely implemented in Cython.

[jdemeyer]

Replying to SimonKing:

Why did sage -git trac push not change the commit field but only the branch field? I changed the commit field manually.

Known bug: ​https://github.com/sagemath/sage_trac_plugin/issues/10

[jdemeyer]

85

[jdemeyer]

Also: please merge 8.1.beta4

[git]

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

​900b33c	Merge branch 'develop' into t/21437/fix-mtx-matrices
​f6bb5bd	Fix wrong comment: libmeataxe is static

[SimonKing]

Done, so "needs review", with the caveat that I didn't run tests again.

[jdemeyer]

One detail:

    ## Define an environment variable that enables MeatAxe to find
    ## its multiplication tables.

It's not an environment variable, it's a string inside the meataxe library.

If you fix this comment, you can set this ticket to positive review.

[git]

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

​3401f04

Fix misleading comment on a global MeatAxe variable

[SimonKing]

Thank you! I hope the comment is fine now.