# Ticket #4983(closed enhancement: fixed)

Opened 4 years ago

## replace subdivisions attribute for matrices with a function

Reported by: Owned by: was was minor sage-4.7 linear algebra N/A Rob Beezer John Palmieri sage-4.7.alpha4

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

Apply:

1. trac_4983-subdivisions-rebased.patch

## Change History

### comment:1 Changed 4 years ago by robertwb

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.

### comment:2 Changed 4 years ago by AlexGhitza

• Type changed from defect to enhancement

### comment:3 Changed 2 years ago by jhpalmieri

• Priority changed from major to minor
• Status changed from new to needs_review
• Report Upstream set to N/A
• Authors set to John Palmieri

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

### comment:4 Changed 2 years ago by rbeezer

• Status changed from needs_review to positive_review
• Reviewers set to Rob Beezer

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.

### comment:5 Changed 2 years ago by jason

This probably conflicts with #10847 too.

### comment:6 Changed 2 years ago by jason

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

### comment:7 Changed 2 years ago by kcrisman

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?

### comment:8 follow-up: ↓ 9 Changed 2 years ago by 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().)

### comment:9 in reply to: ↑ 8 Changed 2 years ago by kcrisman

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.

### comment:10 Changed 2 years ago by jhpalmieri

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.

### comment:11 Changed 2 years ago by jdemeyer

• Status changed from positive_review to needs_work

This patch conflicts with #10974.

### comment:12 follow-up: ↓ 15 Changed 2 years ago by jhpalmieri

• Status changed from needs_work to needs_review

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

### comment:13 Changed 2 years ago by jhpalmieri

• Description modified (diff)

### comment:14 follow-up: ↓ 17 Changed 2 years ago by kcrisman

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.

### comment:15 in reply to: ↑ 12 Changed 2 years ago by rbeezer

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

### comment:16 follow-up: ↓ 18 Changed 2 years ago by jhpalmieri

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

### comment:17 in reply to: ↑ 14 Changed 2 years ago by jdemeyer

• Status changed from needs_review to positive_review

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

Done.

### comment:18 in reply to: ↑ 16 Changed 2 years ago by rbeezer

• Description modified (diff)