Opened 2 years ago

Closed 2 years ago

#23706 closed enhancement (fixed)

allow several implementations of matrices in MatrixSpace

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-8.2
Component: linear algebra Keywords: days88
Cc: jipilab, SimonKing Merged in:
Authors: Vincent Delecroix Reviewers: Jean-Philippe Labbé, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 018226b (Commits) Commit: 018226b9ecafdc082a4eb8289269af85b2908d3b
Dependencies: #24096 Stopgaps:

Description (last modified by vdelecroix)

The aim of this ticket is to revamp the MatrixSpace constructor in order to be able to choose the implementation.

For example, with the attached branch one can have access to 2 implementations of integer matrices

sage: MatrixSpace(ZZ, 3, implementation='flint')
Full MatrixSpace of 3 by 3 dense matrices over Integer Ring
sage: MatrixSpace(ZZ, 3, implementation='generic')
Full MatrixSpace of 3 by 3 dense matrices over Integer Ring (using Matrix_generic_dense)

One important change is the move of the method _get_matrix_class of MatrixSpace as an independent function.

A problem arose with CartanMatrix (in sage/combinat/root_system/cartan_matrix.py). The class CartanMatrix inherits from Matrix_integer_sparse but does not properly redefines the parent. As a consequence of the changes here, many matrix operations are not valid anymore for inherited types (mostly submatrices operations such as __getitem__ with slices or stack). A fix is provided by defining CartanMatrix.matrix_space.

follow up: #23714 (implementation of gap matrices)

Change History (70)

comment:1 Changed 2 years ago by vdelecroix

  • Description modified (diff)

comment:2 Changed 2 years ago by vdelecroix

  • Keywords sd88 added

comment:3 Changed 2 years ago by jipilab

  • Keywords days88 added; sd88 removed

comment:4 Changed 2 years ago by vdelecroix

  • Branch set to u/vdelecroix/23706
  • Commit set to 34f1ddc7e0ea69ab280567bf68da69d5fb2f0cd2

experimental branch ready!


New commits:

457d2fb23704: getitem/setitem for libgap elements
34f1ddcTMP 23706 commit

comment:5 Changed 2 years ago by jipilab

  • Reviewers set to Jean-Philippe Labbé

The title of the ticket could be a bit more specific.

comment:6 Changed 2 years ago by git

  • Commit changed from 34f1ddc7e0ea69ab280567bf68da69d5fb2f0cd2 to d9f0532af4ca79ba467e466c741416b4409ca960

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

d9f0532TMP 23706 commit

comment:7 Changed 2 years ago by git

  • Commit changed from d9f0532af4ca79ba467e466c741416b4409ca960 to 007bdcf7710533d0fd413c56d9d566b84db1873f

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

a92581823704: forgot to set j appropriately!
007bdcfTMP COMMIT 23706: fix some doctests

comment:8 Changed 2 years ago by git

  • Commit changed from 007bdcf7710533d0fd413c56d9d566b84db1873f to f8940d448bddc312f64ad8707cec630abf178b08

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

f8940d4TMP 23706: remove print statement

comment:9 Changed 2 years ago by git

  • Commit changed from f8940d448bddc312f64ad8707cec630abf178b08 to 235c5615b9dd35891a5ff5d598884093b152520b

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

235c561TMP 23706 COMMIT: remove randomize for cyclo_dense

comment:10 Changed 2 years ago by git

  • Commit changed from 235c5615b9dd35891a5ff5d598884093b152520b to dce22c869eee2d2b4a6f59cd9f2e6b5eda633121

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

dce22c8TMP COMMIT 23706: more careful about input

comment:11 Changed 2 years ago by vdelecroix

  • Description modified (diff)
  • Summary changed from gap class for matrices to allow several implementations of matrices in MatrixSpace

comment:12 Changed 2 years ago by vdelecroix

  • Description modified (diff)

comment:13 Changed 2 years ago by git

  • Commit changed from dce22c869eee2d2b4a6f59cd9f2e6b5eda633121 to f1013de2bc8d01830840422c367dbe9224d0aeb6

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

4dc210b23704: forgot to set j appropriately!
c3241d623704: "multi-indices "multi indices" "multiindices"
f1013de23706: allow different matrix implementations

comment:14 Changed 2 years ago by vdelecroix

  • Dependencies set to #23704
  • Status changed from new to needs_review

comment:15 Changed 2 years ago by vdelecroix

  • Description modified (diff)

comment:16 Changed 2 years ago by git

  • Commit changed from f1013de2bc8d01830840422c367dbe9224d0aeb6 to 0cbf7ff5a1e4ec51bfd4251e7a0677b77ec945ce

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

0cbf7ff23706: fix call to _get_matrix_class

comment:17 Changed 2 years ago by vdelecroix

hum... got a lot of failures

combinat/root_system/root_lattice_realizations.py  # 1 doctest failed
combinat/root_system/root_system.py  # 1 doctest failed
combinat/root_system/weight_space.py  # 3 doctests failed
combinat/root_system/type_affine.py  # 3 doctests failed
combinat/root_system/ambient_space.py  # 1 doctest failed
combinat/root_system/root_space.py  # 1 doctest failed
combinat/root_system/cartan_type.py  # 2 doctests failed
categories/crystals.py  # 2 doctests failed
combinat/root_system/weight_lattice_realizations.py  # 12 doctests failed
combinat/root_system/cartan_matrix.py  # 5 doctests failed
combinat/root_system/type_relabel.py  # 1 doctest failed
combinat/root_system/dynkin_diagram.py  # 4 doctests failed
combinat/root_system/type_marked.py  # 1 doctest failed
combinat/root_system/type_reducible.py  # 2 doctests failed

comment:18 Changed 2 years ago by vdelecroix

  • Dependencies changed from #23704 to #23704, #23721
  • Status changed from needs_review to needs_work

Needs to fix #23721 first.

comment:19 Changed 2 years ago by git

  • Commit changed from 0cbf7ff5a1e4ec51bfd4251e7a0677b77ec945ce to 9f74040baa887b1ff6e286cbccb7af77f364c6f3

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

a08706623721: __copy__/__deepcopy__ on GapElement
9f7404023706: allow to choose implementation of matrices

comment:20 Changed 2 years ago by vdelecroix

  • Dependencies #23704, #23721 deleted
  • Description modified (diff)

comment:21 Changed 2 years ago by vdelecroix

  • Status changed from needs_work to needs_review

I postponed the introduction of Matrix_gap in ticket #23714

comment:22 Changed 2 years ago by vdelecroix

  • Status changed from needs_review to needs_work

... wrong commits

comment:23 Changed 2 years ago by git

  • Commit changed from 9f74040baa887b1ff6e286cbccb7af77f364c6f3 to 4efbf513a230daf4a39316149eedea94282ee335

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

4efbf5123706: allow to choose matrix mplementation

comment:24 Changed 2 years ago by vdelecroix

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

comment:25 Changed 2 years ago by vdelecroix

JP, do you feel up for the review?

comment:26 Changed 2 years ago by vdelecroix

Fucking root systems

sage -t --long src/sage/combinat/root_system/cartan_type.py  # 2 doctests failed
sage -t --long src/sage/combinat/root_system/weight_lattice_realizations.py  # 12 doctests failed
sage -t --long src/sage/combinat/root_system/ambient_space.py  # 1 doctest failed
sage -t --long src/sage/combinat/root_system/root_lattice_realizations.py  # 1 doctest failed
sage -t --long src/sage/combinat/root_system/type_marked.py  # 1 doctest failed
sage -t --long src/sage/combinat/root_system/type_reducible.py  # 2 doctests failed
sage -t --long src/sage/combinat/root_system/root_space.py  # 1 doctest failed
sage -t --long src/sage/combinat/root_system/cartan_matrix.py  # 5 doctests failed
sage -t --long src/sage/combinat/root_system/dynkin_diagram.py  # 4 doctests failed
sage -t --long src/sage/combinat/root_system/weight_space.py  # 3 doctests failed
sage -t --long src/sage/combinat/root_system/type_relabel.py  # 1 doctest failed
sage -t --long src/sage/combinat/root_system/type_affine.py  # 3 doctests failed
sage -t --long src/sage/combinat/root_system/root_system.py  # 1 doctest failed
sage -t --long src/sage/categories/crystals.py  # 2 doctests failed

comment:27 Changed 2 years ago by git

  • Commit changed from 4efbf513a230daf4a39316149eedea94282ee335 to d5fa13d8e9c3b44b549873810299b256fa7b6123

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

d5fa13d23706: dirty fixes for Cartan matrices

comment:28 Changed 2 years ago by vdelecroix

  • Description modified (diff)

comment:29 Changed 2 years ago by git

  • Commit changed from d5fa13d8e9c3b44b549873810299b256fa7b6123 to 98d9ae1b64aac938ccb96b1b4f699b44b434bbf5

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

98d9ae123706: fix for CartanMatrix

comment:30 Changed 2 years ago by vdelecroix

Cartan matrices are fixed... waiting for patchbot verdict again.

comment:31 Changed 2 years ago by vdelecroix

  • Description modified (diff)

comment:32 follow-up: Changed 2 years ago by jipilab

Ok, I'm "back on trac"!

I'll give a look at this today.

Where should I throw in the benchmarking script? I guess it is not a good idea to hard code the best implementation, but at least, in the tests, we could add some relevant benchmarks to keep track of the evolution?

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

comment:33 Changed 2 years ago by jipilab

In the description of the test:

+        Check that multiplication for matrices with different backends are not allowed::

I would also mention that sparsity is checked ( the reason why there is no X appearing in row 2, column 3...)

The changes in the CartanMatrix? look good.

In one of the bot's report, I get an error here:

**********************************************************************
File "src/sage/matrix/matrix_gfpn_dense.pyx", line 1484, in sage.matrix.matrix_gfpn_dense.Matrix_gfpn_dense._echelon_in_place_classical
Failed example:
    type(X)
Expected:
    <type 'sage.matrix.matrix_generic_dense.Matrix_generic_dense'>
Got:
    <type 'sage.matrix.matrix_gfpn_dense.Matrix_gfpn_dense'>
**********************************************************************

Other errors look to be unrelated. I'm still looking into the changes.

comment:34 follow-up: Changed 2 years ago by jipilab

I ran the test on the file src/sage/matrix/matrix_gfpn_dense.pyx and all tests passed...

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

comment:35 Changed 2 years ago by jipilab

  • Branch changed from u/vdelecroix/23706 to u/jipilab/23706

comment:36 Changed 2 years ago by jipilab

  • Commit changed from 98d9ae1b64aac938ccb96b1b4f699b44b434bbf5 to ae0bfc33a476dcf8ffe6e14808b4f36c2a2bf9c9

Just did some pep8 corrections... Otherwise, it looks good. Perhaps we should wait to get a proper bot to go over the changes as they touch many files...


New commits:

ae0bfc3Some pep8 conventions

comment:37 in reply to: ↑ 34 Changed 2 years ago by vdelecroix

Replying to jipilab:

I ran the test on the file src/sage/matrix/matrix_gfpn_dense.pyx and all tests passed...

This need to be fixed. It depends whether meataxe is installed or not.

comment:38 Changed 2 years ago by vdelecroix

  • Branch changed from u/jipilab/23706 to public/23706
  • Commit changed from ae0bfc33a476dcf8ffe6e14808b4f36c2a2bf9c9 to cc35c82b9d07517be8af21f67dbca33241cda6ac

New commits:

cc35c8223706: fix doctest in matrix_gfpn_dense.pyx

comment:39 Changed 2 years ago by vdelecroix

Much cleaner now!!

comment:40 in reply to: ↑ 32 Changed 2 years ago by vdelecroix

Replying to jipilab:

Ok, I'm "back on trac"!

I'll give a look at this today.

Where should I throw in the benchmarking script? I guess it is not a good idea to hard code the best implementation, but at least, in the tests, we could add some relevant benchmarks to keep track of the evolution?

Not here. You should open a new ticket (and you can add it to the follow-up list of this ticket). Moreover the concept of "best implementation" depends on what you intend to do (e.g. matrix multiplication, Gauss elimination, determinant, etc).

comment:41 follow-up: Changed 2 years ago by jipilab

The ticket does not apply on beta5. I'm compiling b5 right now and I'll check if I can diagnose further. I have to install meataxe and checkout testing. Give my machine some time and I get back here!

comment:42 follow-up: Changed 2 years ago by jipilab

Is there a dependency to #23721?

comment:43 in reply to: ↑ 42 Changed 2 years ago by vdelecroix

Replying to jipilab:

Is there a dependency to #23721?

Not that I know of...

comment:44 in reply to: ↑ 41 ; follow-up: Changed 2 years ago by vdelecroix

Replying to jipilab:

The ticket does not apply on beta5. I'm compiling b5 right now and I'll check if I can diagnose further. I have to install meataxe and checkout testing. Give my machine some time and I get back here!

The problem comes from something that has changed in matrix_space.py. The merge is simple but I have to leave in 5 minutes... will fix that first thing this afternoon.

comment:45 Changed 2 years ago by git

  • Commit changed from cc35c82b9d07517be8af21f67dbca33241cda6ac to 869b155d053a8f4cb8d69d2575328b2d3ceabc86

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

869b155Rebased on beta5 (attempt to solve conflict)

comment:46 in reply to: ↑ 44 Changed 2 years ago by jipilab

Replying to vdelecroix:

Replying to jipilab:

The ticket does not apply on beta5. I'm compiling b5 right now and I'll check if I can diagnose further. I have to install meataxe and checkout testing. Give my machine some time and I get back here!

The problem comes from something that has changed in matrix_space.py. The merge is simple but I have to leave in 5 minutes... will fix that first thing this afternoon.

Damn, I just read this comment... Hope I did not create any trouble.

comment:47 Changed 2 years ago by vdelecroix

It's ok.

comment:48 Changed 2 years ago by jipilab

There are some spurious doc errors, otherwise, it looks good.

comment:49 Changed 2 years ago by git

  • Commit changed from 869b155d053a8f4cb8d69d2575328b2d3ceabc86 to 079981ab5ca646a65072815b7b956060b908f5f7

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

47a0f1323706: remove trailing whitespaces in cartan_matrix.py
079981a23706: fix doctests

comment:50 Changed 2 years ago by vdelecroix

j'ai poussé... reste plus qu'à attendre les verdicts de madame patchbot.

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

comment:51 Changed 2 years ago by vdelecroix

  • Branch changed from public/23706 to u/vdelecroix/23706
  • Commit changed from 079981ab5ca646a65072815b7b956060b908f5f7 to 5c415f5070e1e4cfb0e57c3c99cdd1470114e2ed

New commits:

dee269a23706: allow to choose matrix implementation
0ca3ae523706: fix for CartanMatrix
4cb0c2fSome pep8 conventions
5c415f523706: fix doctest in matrix_gfpn_dense.pyx

comment:52 Changed 2 years ago by jdemeyer

  • Dependencies set to #24096

Conflicts with #24096

comment:53 Changed 2 years ago by git

  • Commit changed from 5c415f5070e1e4cfb0e57c3c99cdd1470114e2ed to 4f818deff90d1db47f55603509d22f0c267fcf8a

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

08c986523706: allow to choose matrix implementation
c3b537023706: fix for CartanMatrix
07376dfSome pep8 conventions
4f818de23706: fix doctest in matrix_gfpn_dense.pyx

comment:54 Changed 2 years ago by vdelecroix

rebased on 8.1.rc0

comment:55 Changed 2 years ago by vdelecroix

patchbot is happy! jipilab?

comment:56 follow-up: Changed 2 years ago by tscrim

  • Cc SimonKing added
  • Reviewers changed from Jean-Philippe Labbé to Jean-Philippe Labbé, Travis Scrimshaw
  • Status changed from needs_review to needs_work

I have a few comments:

-    def matrix_space(self, nrows=None, ncols=None, sparse=None):
+    def matrix_space(self, nrows=None, ncols=None, sparse=True):
         r"""
         Return a matrix space over the integers.
 
         INPUT:
 
-        - ``nrows`` - number of rows
+        - ``nrows`` -- number of rows
 
-        - ``ncols`` - number of columns
+        - ``ncols`` -- number of columns
 
-        - ``sparse`` - (boolean) sparseness
+        - ``sparse`` -- (boolean) sparseness
 
         EXAMPLES::
 
             sage: cm = CartanMatrix(['A', 3])
             sage: cm.matrix_space()
             Full MatrixSpace of 3 by 3 sparse matrices over Integer Ring
             sage: cm.matrix_space(2, 2)
             Full MatrixSpace of 2 by 2 sparse matrices over Integer Ring
             sage: cm[:2,1:]   # indirect doctest
             [-1  0]
             [ 2 -1]
         """
         if nrows is None:
             nrows = self.nrows()
         if ncols is None:
             ncols = self.ncols()
-        if sparse is None:
-            sparse = True
 
         if nrows == self.nrows() and ncols == self.ncols() and sparse:
             return self.parent()
         else:
             from sage.matrix.matrix_space import MatrixSpace
             return MatrixSpace(ZZ, nrows, ncols, sparse is None or bool(sparse))

I would make this into a more 2/3 compatible test:

            sage: for i in range(3):
            ....:     for j in range(3):
            ....:         try:
            ....:             s = M[i].an_element() * M[j].an_element()
            ....:             print('X', end='')
            ....:         except TypeError:
            ....:             print(' ', end='')
            ....:     print()

I would instead create a 0/1-matrix and output that. Another option would be creating an appropriate string with '\n'.join(...). Similar for another test.

These tests should not be marked as meataxe if it really is using the generic implementation:

            sage: MS = MatrixSpace(M.base_ring(), M.nrows(), M.ncols(), implementation='generic')
            sage: X = MS(M)                         # optional: meataxe
            sage: type(X)                           # optional: meataxe
            sage: X.echelon_form() # optional: meataxe

Otherwise that would be a bug.

Do we want m4ri to have precedence over trying meataxe when no implementation is given? (I'm cc-ing Simon to note that there might be a change in behavior.)

I'm also split on documenting about when we use a non-generic implementation by default. It feels too much like an implementation detail that it should be hidden from the more casual users, but I think it is good for the more advanced users to have it in the public doc. Thoughts?

I'm also not 100% convinced about having to explicitly convert matrices of different implementations, but since things are likely to be incompatible and it seems unlikely that this would be a problem (I think a user who is choosing implementations will probably be careful and might even appreciate this error). I do not have a good reason to make this an objection, but I am just noting that this might come up from a user.

comment:57 Changed 2 years ago by git

  • Commit changed from 4f818deff90d1db47f55603509d22f0c267fcf8a to eeeba324b329c69b8e7c625423cfe4eca4862fc2

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

eeeba3223706: a default argument and doctests

comment:58 in reply to: ↑ 56 ; follow-up: Changed 2 years ago by vdelecroix

Thanks Travis for having a look.

Replying to tscrim:

[SNIP]

agreed and changed in eeeba32

Do we want m4ri to have precedence over trying meataxe when no implementation is given? (I'm cc-ing Simon to note that there might be a change in behavior.)

There should not be any change. It concerns only the case characteristic = 2.

if implementation is None:
    if R.characteristic() == 2 and R.order() <= 65536:
        implementation = 'm4ri'
    elif R.order() <= 255:
        try:
            from . import matrix_gfpn_dense
        except ImportError:
            implementation = 'generic'
        else:
            implementation = 'meataxe'
    else:
        implementation = 'generic'

where it used to be

if R.characteristic() == 2:
    if R.order() <= 65536:
        return matrix_gf2e_dense.Matrix_gf2e_dense
elif R.order() <= 255:
    try:
        from . import matrix_gfpn_dense
        return matrix_gfpn_dense.Matrix_gfpn_dense
    except ImportError:
        pass

I'm also split on documenting about when we use a non-generic implementation by default. It feels too much like an implementation detail that it should be hidden from the more casual users, but I think it is good for the more advanced users to have it in the public doc. Thoughts?

What would you expect exactly? It would be a nightmare to keep the documentation up to date.

I'm also not 100% convinced about having to explicitly convert matrices of different implementations, but since things are likely to be incompatible and it seems unlikely that this would be a problem (I think a user who is choosing implementations will probably be careful and might even appreciate this error). I do not have a good reason to make this an objection, but I am just noting that this might come up from a user.

It should be possible to implement coercion to the default implementation (with conversion). Anyway, I would like to keep such changes for later.

comment:59 Changed 2 years ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:60 in reply to: ↑ 58 Changed 2 years ago by tscrim

  • Status changed from needs_review to positive_review

Replying to vdelecroix:

Thanks Travis for having a look.

Do we want m4ri to have precedence over trying meataxe when no implementation is given? (I'm cc-ing Simon to note that there might be a change in behavior.)

There should not be any change. It concerns only the case characteristic = 2. [snip]

Right, actually I might have misread what meataxe is for and it's a non-issue because it is not meant for characteristic 2. Sorry for the noise.

I'm also split on documenting about when we use a non-generic implementation by default. It feels too much like an implementation detail that it should be hidden from the more casual users, but I think it is good for the more advanced users to have it in the public doc. Thoughts?

What would you expect exactly? It would be a nightmare to keep the documentation up to date.

Keeping it up to date would not be such a problem; at least I don't expect it to change that much. However, I tried a few different ways to present the info and nothing really worked too well. There's just a few too many little cases for the finite fields. I guess if someone cares enough they are good enough to just look at the code.

Thanks. Positive review.

comment:61 Changed 2 years ago by vbraun

  • Status changed from positive_review to needs_work

Documentation doesn't build

comment:62 Changed 2 years ago by git

  • Commit changed from eeeba324b329c69b8e7c625423cfe4eca4862fc2 to 6bff8c84a2cf7013f1bbc1b1a1cf737454a6a888

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

c06ed6e23706: allow to choose matrix implementation
758947e23706: fix for CartanMatrix
e394c15Some pep8 conventions
c16fc7923706: fix doctest in matrix_gfpn_dense.pyx
6bff8c823706: a default argument and doctests

comment:63 Changed 2 years ago by vdelecroix

  • Milestone changed from sage-8.1 to sage-8.2
  • Status changed from needs_work to needs_review

With the rebasing I do not have any problem with the documentation.

comment:64 Changed 2 years ago by vdelecroix

  • Status changed from needs_review to needs_work

found out

[dochtml] [matrices ] docstring of sage.matrix.action.MatrixMatrixAction:24: WARNING: Unexpected indentation.
[dochtml] [matrices ] docstring of sage.matrix.action.MatrixMatrixAction:25: WARNING: Block quote ends without a blank line; unexpected unindent.
[dochtml] [matrices ] /opt/sage/local/lib/python2.7/site-packages/sage/matrix/special.py:docstring of sage.matrix.special:48: WARNING: undefined label: sage.combinat.matrices.hadamard_matrix (if the link has no caption the label must precede a section header)
[dochtml] [matrices ] /opt/sage/local/lib/python2.7/site-packages/sage/matrix/special.py:docstring of sage.matrix.special:49: WARNING: undefined label: sage.combinat.matrices.latin (if the link has no caption the label must precede a section header)
[dochtml] Error building the documentation.
[dochtml] Traceback (most recent call last):
[dochtml]   File "/opt/sage/local/lib/python2.7/runpy.py", line 174, in _run_module_as_main
[dochtml]     "__main__", fname, loader, pkg_name)
[dochtml]   File "/opt/sage/local/lib/python2.7/runpy.py", line 72, in _run_code
[dochtml]     exec code in run_globals
[dochtml]   File "/opt/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__main__.py", line 2, in <module>
[dochtml]     main()
[dochtml]   File "/opt/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 1675, in main
[dochtml]     builder()
[dochtml]   File "/opt/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 310, in _wrapper
[dochtml]     getattr(get_builder(document), 'inventory')(*args, **kwds)
[dochtml]   File "/opt/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 505, in _wrapper
[dochtml]     build_many(build_ref_doc, L)
[dochtml]   File "/opt/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 260, in build_many
[dochtml]     results.append(target(arg))
[dochtml]   File "/opt/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 71, in build_ref_doc
[dochtml]     getattr(ReferenceSubBuilder(doc, lang), format)(*args, **kwds)
[dochtml]   File "/opt/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 720, in _wrapper
[dochtml]     getattr(DocBuilder, build_type)(self, *args, **kwds)
[dochtml]   File "/opt/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 105, in f
[dochtml]     runsphinx()
[dochtml]   File "/opt/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/sphinxbuild.py", line 218, in runsphinx
[dochtml]     raise exception
[dochtml] OSError: [matrices ] docstring of sage.matrix.action.MatrixMatrixAction:24: WARNING: Unexpected indentation.
[dochtml] 
[dochtml] [matrices ] WARNING: Error building the documentation.
[dochtml] [matrices ] Traceback (most recent call last):
[dochtml] [matrices ] File "/opt/sage/local/lib/python2.7/runpy.py", line 174, in _run_module_as_main
[dochtml] [matrices ] "__main__", fname, loader, pkg_name)
[dochtml] [matrices ] File "/opt/sage/local/lib/python2.7/runpy.py", line 72, in _run_code
[dochtml] [matrices ] exec code in run_globals
[dochtml] [matrices ] File "/opt/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__main__.py", line 2, in <module>
[dochtml] [matrices ] main()
[dochtml] [matrices ] File "/opt/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 1675, in main
[dochtml] [matrices ] builder()
[dochtml] [matrices ] File "/opt/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 310, in _wrapper
[dochtml] [matrices ] getattr(get_builder(document), 'inventory')(*args, **kwds)
[dochtml] [matrices ] File "/opt/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 505, in _wrapper
[dochtml] [matrices ] build_many(build_ref_doc, L)
[dochtml] [matrices ] File "/opt/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 260, in build_many
[dochtml] [matrices ] results.append(target(arg))
[dochtml] [matrices ] File "/opt/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 71, in build_ref_doc
[dochtml] [matrices ] getattr(ReferenceSubBuilder(doc, lang), format)(*args, **kwds)
[dochtml] [matrices ] File "/opt/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 720, in _wrapper
[dochtml] [matrices ] getattr(DocBuilder, build_type)(self, *args, **kwds)
[dochtml] [matrices ] File "/opt/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/__init__.py", line 105, in f
[dochtml] [matrices ] runsphinx()
[dochtml] [matrices ] File "/opt/sage/local/lib/python2.7/site-packages/sage_setup/docbuild/sphinxbuild.py", line 218, in runsphinx
[dochtml] [matrices ] raise exception
[dochtml] [matrices ] OSError: [matrices ] docstring of sage.matrix.action.MatrixMatrixAction:24: WARNING: Unexpected indentation.

comment:65 Changed 2 years ago by git

  • Commit changed from 6bff8c84a2cf7013f1bbc1b1a1cf737454a6a888 to 018226b9ecafdc082a4eb8289269af85b2908d3b

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

018226b23706: separate the NOTE and TESTS blocks

comment:66 Changed 2 years ago by vdelecroix

  • Status changed from needs_work to needs_review

I don't understand why, but sphinx was in trouble with NOTE and TESTS being in the same documentation string.

comment:67 Changed 2 years ago by tscrim

It may not be Sphinx but our custom handling of the TESTS block (at least, IIRC we do something special there in order to hide the block).

comment:68 Changed 2 years ago by tscrim

  • Status changed from needs_review to positive_review

Anyways, the workaround is fine with me and there is a green patchbot. Back to positive review.

comment:69 Changed 2 years ago by vdelecroix

thanks Travis.

comment:70 Changed 2 years ago by vbraun

  • Branch changed from u/vdelecroix/23706 to 018226b9ecafdc082a4eb8289269af85b2908d3b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.