Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proper initialisation for the optional MeatAxe library #21437

Closed
simon-king-jena opened this issue Sep 6, 2016 · 157 comments
Closed

Proper initialisation for the optional MeatAxe library #21437

simon-king-jena opened this issue Sep 6, 2016 · 157 comments

Comments

@simon-king-jena
Copy link
Member

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.

Depends on #23411
Depends on #23352

Component: packages: optional

Keywords: MeatAxe, matrix backend

Author: Simon King

Branch/Commit: 3401f04

Reviewer: Jeroen Demeyer

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

@simon-king-jena
Copy link
Member Author

Branch: u/SimonKing/fix-mtx-matrices

@simon-king-jena
Copy link
Member Author

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

@simon-king-jena
Copy link
Member Author

Author: Simon King

@simon-king-jena
Copy link
Member Author

Commit: e861781

@simon-king-jena
Copy link
Member Author

Changed keywords from none to MeatAxe, matrix backend

@simon-king-jena

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 6, 2016

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

4f3d56cFix index check in get_slice

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 6, 2016

Changed commit from e861781 to 4f3d56c

@videlec
Copy link
Contributor

videlec commented Sep 8, 2016

comment:4

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
**********************************************************************

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 8, 2016

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

a4dded9Make new tests optional

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 8, 2016

Changed commit from 4f3d56c to a4dded9

@simon-king-jena
Copy link
Member Author

comment:6

Replying to @videlec:

I guess you forgot some # optional in the doctests

Done!

@simon-king-jena
Copy link
Member Author

comment:7

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 12, 2016

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 12, 2016

Changed commit from a4dded9 to 291f4dc

@videlec
Copy link
Contributor

videlec commented Sep 13, 2016

comment:10

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 14, 2016

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

03ce5c0Fix bad INPUT::/OUTPUT:: blocks

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 14, 2016

Changed commit from 291f4dc to 03ce5c0

@simon-king-jena
Copy link
Member Author

comment:12

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

Replying to @videlec:

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

@simon-king-jena
Copy link
Member Author

comment:118

Replying to @jdemeyer:

Replying to @simon-king-jena:

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 [comment: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.

@simon-king-jena
Copy link
Member Author

Changed branch from u/jdemeyer/fix-mtx-matrices to u/SimonKing/fix-mtx-matrices

@simon-king-jena
Copy link
Member Author

comment:120

Replying to @simon-king-jena:

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.

@simon-king-jena
Copy link
Member Author

Changed commit from 656dc9c to 063ad71

@embray
Copy link
Contributor

embray commented Sep 1, 2017

comment:121

Replying to @jdemeyer:

Replying to @embray:

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

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

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

@jdemeyer
Copy link

jdemeyer commented Sep 1, 2017

comment:122

Replying to @embray:

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

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

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

@jdemeyer
Copy link

jdemeyer commented Sep 4, 2017

comment:123

Replying to @simon-king-jena:

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

Known bug: sagemath/sage_trac_plugin#10

@jdemeyer
Copy link

jdemeyer commented Sep 6, 2017

comment:124

[comment:85]

@jdemeyer
Copy link

jdemeyer commented Sep 6, 2017

Reviewer: Jeroen Demeyer

@jdemeyer
Copy link

jdemeyer commented Sep 6, 2017

comment:125

Also: please merge 8.1.beta4

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 11, 2017

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 11, 2017

Changed commit from 063ad71 to f6bb5bd

@simon-king-jena
Copy link
Member Author

comment:127

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

@jdemeyer
Copy link

comment:128

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 14, 2017

Changed commit from f6bb5bd to 3401f04

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 14, 2017

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

3401f04Fix misleading comment on a global MeatAxe variable

@simon-king-jena
Copy link
Member Author

comment:130

Thank you! I hope the comment is fine now.

@vbraun
Copy link
Member

vbraun commented Sep 15, 2017

Changed branch from u/SimonKing/fix-mtx-matrices to 3401f04

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants