replace subdivisions attribute for matrices with a function

[williamstein]

I do not like this:

sage: sage: a = matrix(ZZ,4,[1, 0, 0, 0, 0, 1, 0, 0, 1, -1, 1, 0, 1, -1, 1, 2])
sage: sage: b=a.jordan_form()
sage: b.subdivisions
([0, 1, 3, 4], [0, 1, 3, 4])
sage: b.subdivisions = 10
sage: b.subdivisions
10

Notice that you can make the subdivisions nonsense because it can be changed.
Also, of course,

sage: b.subdivisions?
...     The Integer class represents arbitrary precision
        integers.  It derives from the Element class, so
[other useless stuff]

I don't like that at all either. I wish that subdivisions were a method with a proper docstring, doctests, etc., and that variable were hidden.

Then one would do:

   sage: b.subdivisions?
   useful stuff (and also it would be in the reference manual)
and
   sage: b.subdivisions()
   ([0, 1, 3, 4], [0, 1, 3, 4])

---

Depends on:

1. Overhaul matrix stack, augment #10974

Apply:

1. attachment: trac_4983-subdivisions-rebased.patch

Component: linear algebra

Author: John Palmieri

Reviewer: Rob Beezer

Merged: sage-4.7.alpha4

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

[robertwb]

comment:1

The method is

sage: b.get_subdivisions()
([1, 3], [1, 3])

but this should probably be changed to have an attribute _subdivisions and a method subdivisions() for consistency.

[jhpalmieri]

comment:3

Here's patch. This keeps get_subdivisions as a synonym for subdivisions.

[jhpalmieri]

Author: John Palmieri

[rbeezer]

comment:4

This looks real good. Passes long tests. I'm glad to see a "get_" go away.

This means I should mildly change #10974, so I'll go make the changes necessary there and have it depend on this.

[rbeezer]

Reviewer: Rob Beezer

[jasongrout]

comment:5

This probably conflicts with #10847 too.

[jasongrout]

comment:6

(where "conflicts" means that #10847 probably needs to be changed after this patch too.)

[kcrisman]

comment:7

It would be nice if a patch that has had positive review for over a week did not have to be rebased for a patch that has had positive review for seven hours. Could this patch not be based on that instead?

[jhpalmieri]

comment:8

kcrisman: I just posted a fix for #10847. (The issue was, the patches at #10847 used the attribute matrix.subdivisions instead of using the method matrix.get_subdivisions().)

[kcrisman]

comment:9

Replying to @jhpalmieri:

kcrisman: I just posted a fix for #10847. (The issue was, the patches at #10847 used the attribute matrix.subdivisions instead of using the method matrix.get_subdivisions().)

Thanks, I appreciate it. I was aware of the incompatibility, just didn't have time to take care of it myself the next few days.

[jhpalmieri]

Attachment: trac_4983-subdivisions.patch.gz

[jhpalmieri]

comment:10

I just uploaded a new patch; the only difference is I added the comment

    # 'get_subdivisions' is kept for backwards compatibility: see #4983. 

right before get_subdivisions = subdivisions.

[jdemeyer]

comment:11

This patch conflicts with #10974.

[jhpalmieri]

comment:12

Here's a patch rebased against #10974. Does this need review or not?

[jhpalmieri]

comment:13

Attachment: trac_4983-subdivisions-rebased.patch.gz

[kcrisman]

comment:14

If it's literally a fairly trivial rebase and nothing changed in terms of testing, I think it would be okay to just post a diff of what had to be rebased (since the patch is fairly large) and set back to positive review. If there are some actual nontrivial changes in the code then I guess someone would have to review it.

[rbeezer]

comment:15

Replying to @jhpalmieri:

Here's a patch rebased against #10974.

Thanks, John.

Does this need review or not?

Normally, I'd say "not." But I have two or three other rebase tasks for later this afternoon, so I can give it a quick test then.

Rob

[jhpalmieri]

comment:16

I just rebased it, I didn't change anything else, but if Rob has time to run tests on it, that would be great. (I've already done this, but it's good to double-check it.)

[jdemeyer]

comment:17

Replying to @kcrisman:

I think it would be okay to [...] set back to positive review.

Done.

[rbeezer]

comment:18

Replying to @jhpalmieri:

(I've already done this, but it's good to double-check it.)

Double-check shows everything is fine on 4.7.alpha3: applies, builds, passes long tests.

Thanks again, John, for sparing me the work on #10974. As a bonus I upgraded the depends/apply block to Jeroen's new formatting. ;-)

[jdemeyer]

Merged: sage-4.7.alpha4