Opened 14 years ago

Closed 11 years ago

replace subdivisions attribute for matrices with a function

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

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

comment:1 Changed 14 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 14 years ago by AlexGhitza

• Type changed from defect to enhancement

comment:3 Changed 11 years ago by jhpalmieri

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

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

comment:4 Changed 11 years ago by rbeezer

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

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 11 years ago by jason

This probably conflicts with #10847 too.

comment:6 Changed 11 years ago by jason

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

comment:7 Changed 11 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 11 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 11 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 11 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 11 years ago by jdemeyer

• Status changed from positive_review to needs_work

This patch conflicts with #10974.

comment:12 follow-up: ↓ 15 Changed 11 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 11 years ago by jhpalmieri

• Description modified (diff)

comment:14 follow-up: ↓ 17 Changed 11 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 11 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 11 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 11 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 11 years ago by rbeezer

• Description modified (diff)