Opened 4 years ago
Closed 3 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, GitHub, GitLab) | Commit: | 3401f04ebf9f89700b24930967e9af8033c24263 |
Dependencies: | #23411 #23352 | Stopgaps: |
Description (last modified by )
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 4 years ago by
- Branch set to u/SimonKing/fix-mtx-matrices
comment:2 Changed 4 years ago by
- 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
comment:3 Changed 4 years ago by
- Commit changed from e861781fcdd710864b01db832a700b13af20218d to 4f3d56c9eef371fa62bc0e1cb8a2a93251f9150a
Branch pushed to git repo; I updated commit sha1. New commits:
4f3d56c | Fix index check in get_slice
|
comment:4 follow-up: ↓ 6 Changed 4 years ago by
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 4 years ago by
- Commit changed from 4f3d56c9eef371fa62bc0e1cb8a2a93251f9150a to a4dded952bba2f5f2ba57022d0817dbb34e81a5a
Branch pushed to git repo; I updated commit sha1. New commits:
a4dded9 | Make new tests optional
|
comment:6 in reply to: ↑ 4 Changed 4 years ago by
comment:7 Changed 4 years ago by
- 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 4 years ago by
- Commit changed from a4dded952bba2f5f2ba57022d0817dbb34e81a5a to 291f4dc68f4588b76cf4eea4ee8135b8ab7523e3
comment:9 Changed 4 years ago by
- Status changed from needs_work to needs_review
comment:10 follow-up: ↓ 12 Changed 4 years ago by
- Status changed from needs_review to needs_info
There are strange doctest errors on arando. Could you have a look?
comment:11 Changed 4 years ago by
- Commit changed from 291f4dc68f4588b76cf4eea4ee8135b8ab7523e3 to 03ce5c01b33ce70069e8fad0c69f5a554fb00f44
Branch pushed to git repo; I updated commit sha1. New commits:
03ce5c0 | Fix bad INPUT::/OUTPUT:: blocks
|
comment:12 in reply to: ↑ 10 Changed 4 years ago by
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 4 years ago by
- Commit changed from 03ce5c01b33ce70069e8fad0c69f5a554fb00f44 to 5ed2269ff4017c5fb76593237b26596324ad1aa0
Branch pushed to git repo; I updated commit sha1. New commits:
5ed2269 | Fix one doctest
|
comment:15 Changed 4 years ago by
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 4 years ago by
- 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: ↓ 23 Changed 4 years ago by
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 4 years ago by
This is deprecated syntax:
for i from 0<=i<self.Data.Nor:
Cython understands range()
.
comment:19 Changed 4 years ago by
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 4 years ago by
Wrong ticket:
deprecation(12345, "Reading this pickle may be machine dependent")
comment:21 Changed 4 years ago by
This function is not used nor tested anywhere:
cdef Matrix_t *rawMatrix(int Field, list entries) except NULL
comment:22 Changed 4 years ago by
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: ↓ 24 Changed 4 years ago by
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.
comment:24 in reply to: ↑ 23 ; follow-ups: ↓ 25 ↓ 40 Changed 4 years ago by
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: ↓ 26 Changed 4 years ago by
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 4 years ago by
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: ↓ 32 Changed 4 years ago by
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: ↓ 29 Changed 4 years ago by
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 4 years ago by
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 4 years ago by
- Commit changed from 5ed2269ff4017c5fb76593237b26596324ad1aa0 to c929fcc3dcde95e6b3037a025491645a3db648ff
comment:31 Changed 4 years ago by
- 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 4 years ago by
Replying to jdemeyer:
There are also some very dubious uses of
sig_on()
insrc/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: ↓ 35 Changed 4 years ago by
- 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 4 years ago by
- Commit changed from c929fcc3dcde95e6b3037a025491645a3db648ff to b70bc5fb54c9329361de06d45f3a052838eae681
Branch pushed to git repo; I updated commit sha1. New commits:
b70bc5f | Change some str into bytes
|
comment:35 in reply to: ↑ 33 Changed 4 years ago by
Replying to SimonKing:
I guess in some places I also have to replace
cdef str
bycdef bytes
...
Done. But I think I should also have a look at the sig_on()
thingy...
comment:36 follow-ups: ↓ 38 ↓ 44 Changed 4 years ago by
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 4 years ago by
- Commit changed from b70bc5fb54c9329361de06d45f3a052838eae681 to 04e7986e6c2113417ced911f827f2ae8247af3aa
Branch pushed to git repo; I updated commit sha1. New commits:
04e7986 | Improve use of sig_on/off/check
|
comment:38 in reply to: ↑ 36 Changed 4 years ago by
- 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 4 years ago by
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): 703 703 [2*z^2 + 2*z + 2 0 0 2*z^2 + z + 2 0 2*z + 1] 704 704 [ 0 2*z^2 + z 0 1 0 2*z^2 + z + 1] 705 705 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 706 712 """ 707 713 self.check_mutability() 708 714 cdef int fl = self.Data.Field … … cdef class Matrix_gfpn_dense(Matrix_dense): 741 747 y = <unsigned char*>x 742 748 for j from 0 <= j < FfCurrentRowSizeIo-1: 743 749 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) )) 745 752 FfStepPtr(&(x)) 746 753 else: 747 754 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: ↓ 45 ↓ 58 Changed 4 years ago by
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 insage.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: ↓ 43 Changed 4 years ago by
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 4 years ago by
- Commit changed from 04e7986e6c2113417ced911f827f2ae8247af3aa to 6a656002aa7d09470707becdb73df00721024fb4
Branch pushed to git repo; I updated commit sha1. New commits:
6a65600 | Use a fixed copy of MtxLibDir
|
comment:43 in reply to: ↑ 41 Changed 4 years ago by
comment:44 in reply to: ↑ 36 ; follow-up: ↓ 47 Changed 4 years ago by
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: ↓ 46 Changed 4 years ago by
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: ↓ 49 Changed 4 years ago by
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 tosrc/sage/libs/meataxe.pyx
. If you setMtxLibDir
insrc/sage/libs/meataxe.pyx
and youimport 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 4 years ago by
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 insidesig_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 4 years ago by
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: ↓ 51 Changed 4 years ago by
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 4 years ago by
22 has not been addressed
comment:51 in reply to: ↑ 49 ; follow-up: ↓ 54 Changed 4 years ago by
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 4 years ago by
- Commit changed from 6a656002aa7d09470707becdb73df00721024fb4 to 67a85cac583d788617b45f5765e0b21498d8b3c2
Branch pushed to git repo; I updated commit sha1. New commits:
67a85ca | Move meataxe definitions from sage.matrix to sage.libs
|
comment:53 Changed 4 years ago by
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: ↓ 56 Changed 4 years ago by
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 4 years ago by
- Status changed from needs_review to needs_work
comment:56 in reply to: ↑ 54 Changed 4 years ago by
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: ↓ 62 Changed 4 years ago by
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 4 years ago by
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: ↓ 60 Changed 4 years ago by
Looking at the MeatAxe? Makefile
, there is no support for building as dynamic library.
comment:60 in reply to: ↑ 59 ; follow-up: ↓ 61 Changed 4 years ago by
comment:61 in reply to: ↑ 60 Changed 4 years ago by
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: ↓ 63 Changed 4 years ago by
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: ↓ 64 ↓ 65 Changed 4 years ago by
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 functioncdef inline init_meataxe(): # set up MtxLibDir and error handlerThen 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 4 years ago by
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: ↓ 67 Changed 4 years ago by
Replying to SimonKing:
Here is a better idea: in
meataxe.pxd
, define an inline functioncdef inline init_meataxe(): # set up MtxLibDir and error handlerThen 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: ↓ 68 Changed 4 years ago by
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: ↓ 70 Changed 4 years ago by
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: ↓ 69 Changed 4 years ago by
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 4 years ago by
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: ↓ 71 Changed 4 years ago by
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
otherwiseMtxLibDir
will be treated as local variable.
comment:71 in reply to: ↑ 70 Changed 4 years ago by
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: ↓ 73 ↓ 74 Changed 4 years ago by
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 4 years ago by
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: ↓ 75 Changed 4 years ago by
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 4 years ago by
comment:76 Changed 4 years ago by
BTW: I would change the name ErrorHandler
to something more descriptive, like sage_meataxe_error_handler
or something.
comment:77 Changed 4 years ago by
- Commit changed from 67a85cac583d788617b45f5765e0b21498d8b3c2 to 353a8b9630ba675342297d9058a38acb1879df39
Branch pushed to git repo; I updated commit sha1. New commits:
353a8b9 | meataxe_init() inline function
|
comment:78 follow-up: ↓ 81 Changed 4 years ago by
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
|
comment:79 Changed 4 years ago by
- Commit changed from 353a8b9630ba675342297d9058a38acb1879df39 to 08e03c2e6c12ea711d23677dbaad13a6c1971821
Branch pushed to git repo; I updated commit sha1. New commits:
08e03c2 | Change ErrorHandler into sage_meataxe_error_handler
|
comment:80 Changed 4 years ago by
- Status changed from needs_work to needs_review
comment:81 in reply to: ↑ 78 Changed 4 years ago by
comment:82 Changed 4 years ago by
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 4 years ago by
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 4 years ago by
- Status changed from needs_review to needs_work
The deprecation ticket number is still wrong.
comment:85 Changed 4 years ago by
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!
- Replace "dynamic" by "static"
- It's not really correct that
meataxe_init()
must be called wheneversage.libs.meataxe
is cimported. It's more correct to say thatmeataxe_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 4 years ago by
- Commit changed from 08e03c2e6c12ea711d23677dbaad13a6c1971821 to b6c8114f9c1480929a9bac9671d9c7b9fa689402
Branch pushed to git repo; I updated commit sha1. New commits:
b6c8114 | Make matrix_gfpn_dense python 3 compliant; fix a deprecation
|
comment:87 Changed 4 years ago by
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:
- 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
- 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
- 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
- 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
|
comment:88 Changed 4 years ago by
- 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 4 years ago by
- Commit changed from b6c8114f9c1480929a9bac9671d9c7b9fa689402 to 1d86d5aebd2b2427a76b91084f7b857c233e163f
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
|
comment:90 Changed 4 years ago by
- Status changed from needs_work to needs_review
comment:91 Changed 4 years ago by
- Description modified (diff)
- Status changed from needs_review to needs_work
comment:92 Changed 4 years ago by
- Commit changed from 1d86d5aebd2b2427a76b91084f7b857c233e163f to 23ebebdf492125dfb289c6efdf32bf240e6b52e7
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
|
comment:93 Changed 4 years ago by
- 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 4 years ago by
- 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 4 years ago by
- Commit changed from 23ebebdf492125dfb289c6efdf32bf240e6b52e7 to 82394bcc093175df3c08e155513fc205a2aa24b6
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
|
comment:96 Changed 4 years ago by
Manual merging of the last few commits of #23352 was needed.
comment:97 Changed 4 years ago by
- Description modified (diff)
comment:98 follow-up: ↓ 99 Changed 4 years ago by
Better use RuntimeError
instead of SystemError
for unknown error messages.
comment:99 in reply to: ↑ 98 ; follow-up: ↓ 100 Changed 4 years ago by
Replying to jdemeyer:
Better use
RuntimeError
instead ofSystemError
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: ↓ 101 ↓ 103 Changed 4 years ago by
Replying to SimonKing:
Replying to jdemeyer:
Better use
RuntimeError
instead ofSystemError
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 4 years ago by
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 ofIOError
becauseIOError
is just an alias ofOSError
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 4 years ago by
- Commit changed from 82394bcc093175df3c08e155513fc205a2aa24b6 to 6cba43e0d93249ae33efb8284b53d9b7d1f1539a
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
|
comment:103 in reply to: ↑ 100 ; follow-up: ↓ 104 Changed 4 years ago by
Replying to jdemeyer:
Replying to SimonKing:
Replying to jdemeyer:
Better use
RuntimeError
instead ofSystemError
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 ofIOError
becauseIOError
is just an alias ofOSError
in Python 3.
Done and done!
comment:104 in reply to: ↑ 103 Changed 4 years ago by
Replying to SimonKing:
Replying to jdemeyer:
Replying to SimonKing:
Replying to jdemeyer:
Better use
RuntimeError
instead ofSystemError
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 ofIOError
becauseIOError
is just an alias ofOSError
in Python 3.Done and done!
I think I have addressed all objections...
comment:105 Changed 4 years ago by
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 4 years ago by
Viewing the diff of this ticket fails with HTTP error 500. I just sent an email to sagemath-admins
for this.
comment:107 Changed 4 years ago by
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.
comment:108 Changed 4 years ago by
- Branch changed from u/SimonKing/fix-mtx-matrices to u/jdemeyer/fix-mtx-matrices
comment:109 Changed 4 years ago by
- Commit changed from 6cba43e0d93249ae33efb8284b53d9b7d1f1539a to 656dc9c926b02ea776fe63f6c88e77f8c85569c5
comment:110 Changed 4 years ago by
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 4 years ago by
- 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: ↓ 113 ↓ 114 Changed 4 years ago by
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 4 years ago by
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: ↓ 121 Changed 4 years ago by
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: ↓ 116 Changed 4 years ago by
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: ↓ 117 Changed 4 years ago by
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: ↓ 118 Changed 4 years ago by
comment:118 in reply to: ↑ 117 ; follow-up: ↓ 120 Changed 4 years ago by
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 4 years ago by
- Branch changed from u/jdemeyer/fix-mtx-matrices to u/SimonKing/fix-mtx-matrices
comment:120 in reply to: ↑ 118 ; follow-up: ↓ 123 Changed 4 years ago by
- 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: ↓ 122 Changed 4 years ago by
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
tobytes
. 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 4 years ago by
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 3 years ago by
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 3 years ago by
- Reviewers set to Jeroen Demeyer
- Status changed from needs_review to needs_work
comment:125 Changed 3 years ago by
Also: please merge 8.1.beta4
comment:126 Changed 3 years ago by
- Commit changed from 063ad71f3d1e49171e821440dbcb2af226499cfd to f6bb5bd4127ec07e5d315105d68cf70c0f4612da
comment:127 Changed 3 years ago by
- 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 3 years ago by
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 3 years ago by
- Commit changed from f6bb5bd4127ec07e5d315105d68cf70c0f4612da to 3401f04ebf9f89700b24930967e9af8033c24263
Branch pushed to git repo; I updated commit sha1. New commits:
3401f04 | Fix misleading comment on a global MeatAxe variable
|
comment:130 Changed 3 years ago by
- Status changed from needs_review to positive_review
Thank you! I hope the comment is fine now.
comment:131 Changed 3 years ago by
- Branch changed from u/SimonKing/fix-mtx-matrices to 3401f04ebf9f89700b24930967e9af8033c24263
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Fix reading mtx-matrix from non-existent file
Help using MeatAxe executables in Sage
Make pickling machine independent
Add c(p)def functions to get/set horizontal matrix slices