Allow tab completion of matrix constructor

[robertwb]

E.g. matrix.identity(...), matrix.random(...), matrix.load(...) etc. all discoverable by tab completion.

Apply attachment: 13678-matrix-methods.v3.patch and attachment: 13678-doctests.patch to the Sage library and attachment: 13678-sagenb.patch to sagenb.

Depends on #13717

CC: @sagetrac-Bouillaguet

Component: linear algebra

Author: Robert Bradshaw, Volker Braun

Reviewer: Volker Braun

Merged: sage-5.7.beta0

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

[robertwb]

comment:1

Attachment: 13678-matrix-methods.patch.gz

[1d7ec08f-60ae-4512-91a6-8324c06eab9f]

comment:2

Very nice! I discovered a couple constructors I didn't know about (just reading the patch). I'll add it to my queue of things to review, but anybody else should feel free to beat me to it.

[1d7ec08f-60ae-4512-91a6-8324c06eab9f]

comment:3

I like it! Works as advertised. I'll run tests shortly. In the meantime, a suggestion and a question.

(1) Stripping out "matrix" from the name of the method is a nice touch, but then the docstring appears (to the novice) to be talking about a slightly diffferent command than the one queried. Having ready access to constructors is the first step in experimenting with Sage, then tab-completion takes over for methods on that object. So tab-completion to discover constructors is fabulous. But it should be as totally straightforward as possible, IMHO.

What do you think of putting "matrix" back in the method names of the matrix object? Yes, it is verbose and redundant. It does not seem to complicate tab-completion (ie, you do not need to use tab any more in either scenario). And in the Sage library, authors can use the "old" method names without involving the matrix object (and I would think this would be preferable).

(2) Is there anyway to make the func_* methods on this object invisible on tab-completion? I have no real good idea if they are useful, or just detritus. It'd be great if matrix.<tab> only showed the constructors.

Rob

[robertwb]

comment:4

Both valid points of feedback.

(1) I'd lean towards not putting matrix back in the name; what if we modified some of the examples to use the new format as well? Would that make things clear that we're talking about the "same function." We could also (automatically) add a line in the docstring that "This function is available as matrix.identity or identity_matrix"

(2) Short of hiding them for all functions (-1 to that) another option is to matrix() a __call__ method on a class. If you want I can post a new patch doing this.

[vbraun]

Attachment: 13678-matrix-methods_vb.patch.gz

Improved patch

[vbraun]

comment:5

Great idea to use a decorator! I've implemented the __call__ version. Also, I gave the decorator an optional name= argument if you want to override the automatic name generation. But I'm happy with the matrix-removed names. I agree that it would be better if matrix.foo were then mentioned in the docstring of foo_matrix, feel free to fix this. But its not really necessary. I'm giving positive review to Robert's patch ;-)

[vbraun]

Author: Robert Bradshaw, Volker Braun

[vbraun]

Description changed:

--- 
+++ 
@@ -1 +1,3 @@
 E.g. matrix.identity(...), matrix.random(...), matrix.load(...) etc. all discoverable by tab completion.
+
+Apply [attachment: 13678-matrix-methods_vb.patch](https://github.com/sagemath/sage-prod/files/10656572/13678-matrix-methods_vb.patch.gz)

[vbraun]

Reviewer: Volker Braun

[1d7ec08f-60ae-4512-91a6-8324c06eab9f]

comment:6

Replying to @vbraun:

I agree that it would be better if matrix.foo were then mentioned in the docstring of foo_matrix, feel free to fix this.

If it is easy to inject something like "matrix.foo() is equivalent to foo_matrix()," could this be added?

It drives me nuts when we define an alias for some method and it is not included in the docstring of the original. I'd put this in the same category, so if a one-time hunk of code takes care of it, then I'd love to see that happen. Just trying to keep an eye out for the newcomers and minimize unnecessary confusion when there are two ways to do things.

[robertwb]

Attachment: 13678-matrix-methods.v3.patch.gz

More improvements.

[robertwb]

comment:7

OK, I added automatic mention of the two forms in the docstring, as well as slight simplification to Volkers patch and showing the actual constructor code for ?? (which I've actually used before).

[robertwb]

Description changed:

--- 
+++ 
@@ -1,3 +1,3 @@
 E.g. matrix.identity(...), matrix.random(...), matrix.load(...) etc. all discoverable by tab completion.

-Apply [attachment: 13678-matrix-methods_vb.patch](https://github.com/sagemath/sage-prod/files/10656572/13678-matrix-methods_vb.patch.gz)
+Apply [attachment: 13678-matrix-methods.v3.patch](https://github.com/sagemath/sage-prod/files/10656573/13678-matrix-methods.v3.patch.gz)

[robertwb]

comment:8

APply only 13678-matrix-methods.v3.patch

[jasongrout]

comment:9

A few more examples of special matrices are up at #13703

[jasongrout]

comment:10

Gee, it seems like it would be nice to generalize the decorator one more level, so I could do something like:

@namespace('matrix')
def vandermonde(R, v):
    return matrix(R, len(v), lambda i,j: v[i]^j)

I could see this being useful for other situations like this too.

@namespace('groups')
def alternating(...):
    ....

[vbraun]

comment:11

There are a few failing doctests; sage_getsource and sage_getdoc get confused and interact(matrix) needs fixing.

[robertwb]

Attachment: 13678-doctests.patch.gz

Apply to sagenb repo.

[robertwb]

comment:13

Attachment: 13678-sagenb.patch.gz

[robertwb]

Description changed:

--- 
+++ 
@@ -1,3 +1,3 @@
 E.g. matrix.identity(...), matrix.random(...), matrix.load(...) etc. all discoverable by tab completion.

-Apply [attachment: 13678-matrix-methods.v3.patch](https://github.com/sagemath/sage-prod/files/10656573/13678-matrix-methods.v3.patch.gz)
+Apply [attachment: 13678-matrix-methods.v3.patch](https://github.com/sagemath/sage-prod/files/10656573/13678-matrix-methods.v3.patch.gz) and [attachment: 13678-doctests.patch](https://github.com/sagemath/sage-prod/files/10656574/13678-doctests.patch.gz) to the Sage library and [attachment: 13678-sagenb.patch](https://github.com/sagemath/sage-prod/files/10656575/13678-sagenb.patch.gz) to sagenb.

[89f39f15-88e8-4e79-9bc0-0739a7fc497c]

comment:14

Looks good to me. The interface of matrix.random_unimodular is a bit weird (and different from that of matrix.random, but this is not this ticket's fault.

[jasongrout]

comment:15

Maybe this and the QQ^(3,2) syntax should be added to the HLA article? What do you think, Robert?

[vbraun]

comment:16

Whats HLA?

[jasongrout]

comment:17

Handbook of Linear Algebra. We were writing an article for HLA on Sage, and the last call for updates just went out. It might make sense for us to put these things in the Sage chapter.

[jdemeyer]

comment:18

This obviously needs a sagenb upgrade.

[jdemeyer]

Dependencies: sagenb-???

[robertwb]

comment:19

Thanks for the review.

I think it'd be worth mentioning matrix.[tab] in HLA, if it's not to late to slip in a note (and this actually goes in in time).

FWIW, sagenb pull request at https://github.com/sagemath/sagenb/pull/125

[kini]

Changed dependencies from sagenb-??? to #13717

[jdemeyer]

Merged: sage-5.7.beta0