Opened 3 years ago

Closed 2 years ago

#21437 closed defect (fixed)

Proper initialisation for the optional MeatAxe library

Reported by: SimonKing Owned by:
Priority: major Milestone: sage-7.6
Component: packages: optional Keywords: MeatAxe, matrix backend
Cc: Merged in:
Authors: Simon King Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 3401f04 (Commits) Commit: 3401f04ebf9f89700b24930967e9af8033c24263
Dependencies: #23411 #23352 Stopgaps:

Description (last modified by jdemeyer)

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.

Change History (131)

comment:1 Changed 3 years ago by SimonKing

  • Branch set to u/SimonKing/fix-mtx-matrices

comment:2 Changed 3 years ago by SimonKing

  • Authors set to Simon King
  • Commit set to e861781fcdd710864b01db832a700b13af20218d
  • Component changed from PLEASE CHANGE to packages: optional
  • Description modified (diff)
  • Keywords MeatAxe matrix backend added
  • Status changed from new to needs_review
  • Type changed from PLEASE CHANGE to defect

New commits:

9e0f5d4Fix reading mtx-matrix from non-existent file
778df7dHelp using MeatAxe executables in Sage
02fa8a2Make pickling machine independent
e861781Add c(p)def functions to get/set horizontal matrix slices

comment:3 Changed 3 years ago by git

  • Commit changed from e861781fcdd710864b01db832a700b13af20218d to 4f3d56c9eef371fa62bc0e1cb8a2a93251f9150a

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

4f3d56cFix index check in get_slice

comment:4 follow-up: Changed 3 years ago by 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
**********************************************************************

comment:5 Changed 3 years ago by git

  • Commit changed from 4f3d56c9eef371fa62bc0e1cb8a2a93251f9150a to a4dded952bba2f5f2ba57022d0817dbb34e81a5a

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

a4dded9Make new tests optional

comment:6 in reply to: ↑ 4 Changed 3 years ago by SimonKing

Replying to vdelecroix:

I guess you forgot some # optional in the doctests

Done!

comment:7 Changed 3 years ago by SimonKing

  • Status changed from needs_review to needs_work

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.

comment:8 Changed 3 years ago by git

  • Commit changed from a4dded952bba2f5f2ba57022d0817dbb34e81a5a to 291f4dc68f4588b76cf4eea4ee8135b8ab7523e3

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

e17e4dfDocument and test _rowlist_()
291f4dcSome minor tweaks for performance

comment:9 Changed 3 years ago by SimonKing

  • Status changed from needs_work to needs_review

comment:10 follow-up: Changed 3 years ago by vdelecroix

  • Status changed from needs_review to needs_info

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

comment:11 Changed 3 years ago by git

  • Commit changed from 291f4dc68f4588b76cf4eea4ee8135b8ab7523e3 to 03ce5c01b33ce70069e8fad0c69f5a554fb00f44

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

03ce5c0Fix bad INPUT::/OUTPUT:: blocks

comment:12 in reply to: ↑ 10 Changed 3 years ago by 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...

comment:13 Changed 3 years ago by git

  • Commit changed from 03ce5c01b33ce70069e8fad0c69f5a554fb00f44 to 5ed2269ff4017c5fb76593237b26596324ad1aa0

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

5ed2269Fix one doctest

comment:14 Changed 3 years ago by SimonKing

  • Status changed from needs_info to needs_review

Fixed!

comment:15 Changed 3 years ago by 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.

comment:16 Changed 3 years ago by jdemeyer

  • Milestone changed from sage-7.4 to sage-7.6
  • Status changed from needs_review to needs_work

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):

comment:17 in reply to: ↑ description ; follow-up: Changed 3 years ago by 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?

comment:18 Changed 3 years ago by jdemeyer

This is deprecated syntax:

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

Cython understands range().

comment:19 Changed 3 years ago by 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)

comment:20 Changed 3 years ago by jdemeyer

Wrong ticket:

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

comment:21 Changed 3 years ago by jdemeyer

This function is not used nor tested anywhere:

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

comment:22 Changed 3 years ago by 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

comment:23 in reply to: ↑ 17 ; follow-up: Changed 3 years ago by 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.

Last edited 3 years ago by SimonKing (previous) (diff)

comment:24 in reply to: ↑ 23 ; follow-ups: Changed 3 years ago by 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.

comment:25 in reply to: ↑ 24 ; follow-up: Changed 3 years ago by 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.

comment:26 in reply to: ↑ 25 Changed 3 years ago by 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.

comment:27 follow-up: Changed 3 years ago by 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.

comment:28 follow-up: Changed 2 years ago by 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?

comment:29 in reply to: ↑ 28 Changed 2 years ago by 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.

comment:30 Changed 2 years ago by git

  • Commit changed from 5ed2269ff4017c5fb76593237b26596324ad1aa0 to c929fcc3dcde95e6b3037a025491645a3db648ff

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

d4dc794Merge branch 'develop' into t/21437/fix-mtx-matrices
413e4a9Remove deprecated Cython syntax, use check_malloc
d943f93Remove rawMatrix function
c929fccRemove unused MTX* environment variables

comment:31 Changed 2 years ago by SimonKing

  • Status changed from needs_work to needs_review

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?

comment:32 in reply to: ↑ 27 Changed 2 years ago by 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?

comment:33 follow-up: Changed 2 years ago by SimonKing

  • Status changed from needs_review to needs_work

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

comment:34 Changed 2 years ago by git

  • Commit changed from c929fcc3dcde95e6b3037a025491645a3db648ff to b70bc5fb54c9329361de06d45f3a052838eae681

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

b70bc5fChange some str into bytes

comment:35 in reply to: ↑ 33 Changed 2 years ago by 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...

comment:36 follow-ups: Changed 2 years ago by 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?

comment:37 Changed 2 years ago by git

  • Commit changed from b70bc5fb54c9329361de06d45f3a052838eae681 to 04e7986e6c2113417ced911f827f2ae8247af3aa

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

04e7986Improve use of sig_on/off/check

comment:38 in reply to: ↑ 36 Changed 2 years ago by SimonKing

  • Status changed from needs_work to needs_review

Replying to SimonKing:

Did I describe the intended use of sig_* correctly?

I hope that the last commit did improve the situation.

comment:39 Changed 2 years ago by 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

    diff --git a/src/sage/matrix/matrix_gfpn_dense.pyx b/src/sage/matrix/matrix_gfpn_dense.pyx
    index 7b5fac9..a5b4ecb 100644
    a b cdef class Matrix_gfpn_dense(Matrix_dense): 
    703703            [2*z^2 + 2*z + 2               0               0   2*z^2 + z + 2               0         2*z + 1]
    704704            [              0       2*z^2 + z               0               1               0   2*z^2 + z + 1]
    705705
     706        The following tests against a bug that was fixed in :trac:`23352`::
     707
     708            sage: MS = MatrixSpace(GF(9,'x'),1,5)
     709            sage: MS.random_element()     # optional: meataxe
     710            [x + 1     x     2 x + 2 x + 2]
     711
    706712        """
    707713        self.check_mutability()
    708714        cdef int fl = self.Data.Field
    cdef class Matrix_gfpn_dense(Matrix_dense): 
    741747                        y = <unsigned char*>x
    742748                        for j from 0 <= j < FfCurrentRowSizeIo-1:
    743749                            y[j] = randint()%O
    744                         y[FfCurrentRowSizeIo-1] = randint()%(fl**(nc%MPB))
     750                        for j from nc-(nc%MPB) <= j < nc:
     751                            FfInsert(x, j, FfFromInt( (randint()%fl) ))
    745752                        FfStepPtr(&(x))
    746753                else:
    747754                    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.

comment:40 in reply to: ↑ 24 ; follow-ups: Changed 2 years ago by 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.

comment:41 follow-up: Changed 2 years ago by 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?

comment:42 Changed 2 years ago by git

  • Commit changed from 04e7986e6c2113417ced911f827f2ae8247af3aa to 6a656002aa7d09470707becdb73df00721024fb4

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

6a65600Use a fixed copy of MtxLibDir

comment:43 in reply to: ↑ 41 Changed 2 years ago by 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.

comment:44 in reply to: ↑ 36 ; follow-up: Changed 2 years ago by 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.

comment:45 in reply to: ↑ 40 ; follow-up: Changed 2 years ago by 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.

comment:46 in reply to: ↑ 45 ; follow-up: Changed 2 years ago by 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?

comment:47 in reply to: ↑ 44 Changed 2 years ago by 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?

comment:48 Changed 2 years ago by 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.

comment:49 in reply to: ↑ 46 ; follow-up: Changed 2 years ago by 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).

comment:50 Changed 2 years ago by jdemeyer

22 has not been addressed

comment:51 in reply to: ↑ 49 ; follow-up: Changed 2 years ago by 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.

comment:52 Changed 2 years ago by git

  • Commit changed from 6a656002aa7d09470707becdb73df00721024fb4 to 67a85cac583d788617b45f5765e0b21498d8b3c2

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

67a85caMove meataxe definitions from sage.matrix to sage.libs

comment:53 Changed 2 years ago by 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.

comment:54 in reply to: ↑ 51 ; follow-up: Changed 2 years ago by 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?

comment:55 Changed 2 years ago by SimonKing

  • Status changed from needs_review to needs_work

comment:56 in reply to: ↑ 54 Changed 2 years ago by 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

comment:57 follow-up: Changed 2 years ago by 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.

comment:58 in reply to: ↑ 40 Changed 2 years ago by 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.

comment:59 follow-up: Changed 2 years ago by jdemeyer

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

comment:60 in reply to: ↑ 59 ; follow-up: Changed 2 years ago by 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.

comment:61 in reply to: ↑ 60 Changed 2 years ago by 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.

comment:62 in reply to: ↑ 57 ; follow-up: Changed 2 years ago by 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.

comment:63 in reply to: ↑ 62 ; follow-ups: Changed 2 years ago by 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.

comment:64 in reply to: ↑ 63 Changed 2 years ago by 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.

comment:65 in reply to: ↑ 63 ; follow-up: Changed 2 years ago by 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.

comment:66 follow-up: Changed 2 years ago by 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.

comment:67 in reply to: ↑ 65 ; follow-up: Changed 2 years ago by 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.

comment:68 in reply to: ↑ 66 ; follow-up: Changed 2 years ago by 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.

comment:69 in reply to: ↑ 68 Changed 2 years ago by 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.

comment:70 in reply to: ↑ 67 ; follow-up: Changed 2 years ago by 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.

comment:71 in reply to: ↑ 70 Changed 2 years ago by jdemeyer

Replying to SimonKing:

Is "global" needed, then?

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

comment:72 follow-ups: Changed 2 years ago by 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?

comment:73 in reply to: ↑ 72 Changed 2 years ago by 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.

comment:74 in reply to: ↑ 72 ; follow-up: Changed 2 years ago by 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)

comment:75 in reply to: ↑ 74 Changed 2 years ago by 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!

comment:76 Changed 2 years ago by jdemeyer

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

comment:77 Changed 2 years ago by git

  • Commit changed from 67a85cac583d788617b45f5765e0b21498d8b3c2 to 353a8b9630ba675342297d9058a38acb1879df39

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

353a8b9meataxe_init() inline function

comment:78 follow-up: Changed 2 years ago by 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:

353a8b9meataxe_init() inline function

comment:79 Changed 2 years ago by git

  • Commit changed from 353a8b9630ba675342297d9058a38acb1879df39 to 08e03c2e6c12ea711d23677dbaad13a6c1971821

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

08e03c2Change ErrorHandler into sage_meataxe_error_handler

comment:80 Changed 2 years ago by SimonKing

  • Status changed from needs_work to needs_review

comment:81 in reply to: ↑ 78 Changed 2 years ago by 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.

comment:82 Changed 2 years ago by 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.

comment:83 in reply to: ↑ description Changed 2 years ago by 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.

comment:84 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work

The deprecation ticket number is still wrong.

comment:85 Changed 2 years ago by 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"
  1. 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.

comment:86 Changed 2 years ago by git

  • Commit changed from 08e03c2e6c12ea711d23677dbaad13a6c1971821 to b6c8114f9c1480929a9bac9671d9c7b9fa689402

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

b6c8114Make matrix_gfpn_dense python 3 compliant; fix a deprecation

comment:87 Changed 2 years ago by 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:

b6c8114Make matrix_gfpn_dense python 3 compliant; fix a deprecation

comment:88 Changed 2 years ago by SimonKing

  • Dependencies set to #23352 #23399 #23400
  • Description modified (diff)
  • Summary changed from Some additions to the optional MeatAxe wrapper to Proper initialisation for the optional MeatAxe library

comment:89 Changed 2 years ago by git

  • Commit changed from b6c8114f9c1480929a9bac9671d9c7b9fa689402 to 1d86d5aebd2b2427a76b91084f7b857c233e163f

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

225afe1Properly fill the last few columns of random meataxe matrices
f6722beAdd c(p)def functions to get/set horizontal matrix slices
6f7caf9Document and test _rowlist_()
bd1f3f1Fix ticket number in deprecation warning
35b4ee7Merge branch 't/23352/meataxe_fixes' into t/23399/some_additions_to_matrix_mod_gfpn_dense
dac9f37Replace 'for...from' by '...in range()'
b97839cChange some str into bytes
6b6642cImprove use of sig_check/on/off
0fb429bMake matrix_gfpn_dense Python 3 compliant
1d86d5aRemove MTX* environment variables, move MeatAxe code to sage.libs

comment:90 Changed 2 years ago by SimonKing

  • Status changed from needs_work to needs_review

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

comment:91 Changed 2 years ago by SimonKing

  • Description modified (diff)
  • Status changed from needs_review to needs_work

comment:92 Changed 2 years ago by git

  • Commit changed from 1d86d5aebd2b2427a76b91084f7b857c233e163f to 23ebebdf492125dfb289c6efdf32bf240e6b52e7

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

4960a81Fix reading mtx-matrix from non-existent file
4e85fcaMake pickling of matrix_gfpn_dense machine independent
f9d878aAdd test for reading from non-existing file; add cimports
bdfb799Merge branch 't/23411/fix_pickling_of_matrix_gfpn_dense' into t/21437/meataxe_initialisation_merge
bbd6a61Properly fill last column of random matrix_gfpn_dense
6bc9168Merge branch 't/23352/fix_random_matrix_gfpn_dense' into t/21437/meataxe_initialisation_merge
23ebebdAdd MeatAxe initialisation function

comment:93 Changed 2 years ago by SimonKing

  • Dependencies changed from #23352 #23399 #23400 to #23410 #23411 #23352
  • Status changed from needs_work to needs_review

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

comment:94 Changed 2 years ago by SimonKing

  • Dependencies changed from #23410 #23411 #23352 to #23411 #23352

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

comment:95 Changed 2 years ago by git

  • Commit changed from 23ebebdf492125dfb289c6efdf32bf240e6b52e7 to 82394bcc093175df3c08e155513fc205a2aa24b6

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

e477948Test meataxe unpickling being machine independent
01e2c0dMark new meataxe test 'optional'
d174835Check bounds when unpickling meataxe matrices
8873330Mark further meataxe tests optional; fix potential zero division
7a1ed5aMeataxe unpickling: Further consistency checks
1c0dcd5Merge branch 't/23411/fix_pickling_of_matrix_gfpn_dense' into t/23400/cleanup_of_matrix_gfpn_dense
71abd44Speedup meataxe matrix randomisation
e68140eReplace sig_on/off by sig_check in meataxe matrix randomisation
82394bcMerge branch 't/23352/fix_random_matrix_gfpn_dense' into t/23400/cleanup_of_matrix_gfpn_dense

comment:96 Changed 2 years ago by SimonKing

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

comment:97 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:98 follow-up: Changed 2 years ago by jdemeyer

Better use RuntimeError instead of SystemError for unknown error messages.

comment:99 in reply to: ↑ 98 ; follow-up: Changed 2 years ago by 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).

comment:100 in reply to: ↑ 99 ; follow-ups: Changed 2 years ago by 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.

comment:101 in reply to: ↑ 100 Changed 2 years ago by 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.

comment:102 Changed 2 years ago by git

  • Commit changed from 82394bcc093175df3c08e155513fc205a2aa24b6 to 6cba43e0d93249ae33efb8284b53d9b7d1f1539a

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

f274bebMeataxe unpickling: Further corner cases
6586e97Sanitise meataxe matrix initialisation
77bc58bFix indentation in meataxe matrix __init__ doc
5106ebbSimplified code path for uninitialised meataxe matrices
3d94133Remove codepath for creating uninitialised meataxe matrix
564104eDon't use six.string_types, but basestring
982755fWorkaround: MeatAxe's MatLoad returns NULL on empty filename without setting error
75ab906Merge branch 't/23411/fix_pickling_of_matrix_gfpn_dense' into t/21437/meataxe_initialisation
6cba43eImprove handling of different meataxe error types

comment:103 in reply to: ↑ 100 ; follow-up: Changed 2 years ago by 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!

comment:104 in reply to: ↑ 103 Changed 2 years ago by 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...

comment:105 Changed 2 years ago by 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.

comment:106 Changed 2 years ago by jdemeyer

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

comment:107 Changed 2 years ago by 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.

Last edited 2 years ago by embray (previous) (diff)

comment:108 Changed 2 years ago by jdemeyer

  • Branch changed from u/SimonKing/fix-mtx-matrices to u/jdemeyer/fix-mtx-matrices

comment:109 Changed 2 years ago by jdemeyer

  • Commit changed from 6cba43e0d93249ae33efb8284b53d9b7d1f1539a to 656dc9c926b02ea776fe63f6c88e77f8c85569c5

I just did that.


New commits:

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

comment:110 Changed 2 years ago by 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.

comment:111 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work

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).

comment:112 follow-ups: Changed 2 years ago by 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?

comment:113 in reply to: ↑ 112 Changed 2 years ago by 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.

comment:114 in reply to: ↑ 112 ; follow-up: Changed 2 years ago by 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

comment:115 follow-up: Changed 2 years ago by 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?

comment:116 in reply to: ↑ 115 ; follow-up: Changed 2 years ago by 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?

comment:117 in reply to: ↑ 116 ; follow-up: Changed 2 years ago by 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

comment:118 in reply to: ↑ 117 ; follow-up: Changed 2 years ago by 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.

comment:119 Changed 2 years ago by SimonKing

  • Branch changed from u/jdemeyer/fix-mtx-matrices to u/SimonKing/fix-mtx-matrices

comment:120 in reply to: ↑ 118 ; follow-up: Changed 2 years ago by SimonKing

  • Commit changed from 656dc9c926b02ea776fe63f6c88e77f8c85569c5 to 063ad71f3d1e49171e821440dbcb2af226499cfd
  • Status changed from needs_work to needs_review

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.

comment:121 in reply to: ↑ 114 ; follow-up: Changed 2 years ago by 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.

comment:122 in reply to: ↑ 121 Changed 2 years ago by 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.

comment:123 in reply to: ↑ 120 Changed 2 years ago by 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

comment:124 Changed 2 years ago by jdemeyer

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

85

comment:125 Changed 2 years ago by jdemeyer

Also: please merge 8.1.beta4

comment:126 Changed 2 years ago by git

  • Commit changed from 063ad71f3d1e49171e821440dbcb2af226499cfd to f6bb5bd4127ec07e5d315105d68cf70c0f4612da

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

900b33cMerge branch 'develop' into t/21437/fix-mtx-matrices
f6bb5bdFix wrong comment: libmeataxe is static

comment:127 Changed 2 years ago by SimonKing

  • Status changed from needs_work to needs_review

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

comment:128 Changed 2 years ago by 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.

comment:129 Changed 2 years ago by git

  • Commit changed from f6bb5bd4127ec07e5d315105d68cf70c0f4612da to 3401f04ebf9f89700b24930967e9af8033c24263

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

3401f04Fix misleading comment on a global MeatAxe variable

comment:130 Changed 2 years ago by SimonKing

  • Status changed from needs_review to positive_review

Thank you! I hope the comment is fine now.

comment:131 Changed 2 years ago by vbraun

  • Branch changed from u/SimonKing/fix-mtx-matrices to 3401f04ebf9f89700b24930967e9af8033c24263
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.