Opened 11 years ago

Closed 8 years ago

#4983 closed enhancement (fixed)

replace subdivisions attribute for matrices with a function

Reported by: was Owned by: was
Priority: minor Milestone: sage-4.7
Component: linear algebra Keywords:
Cc: Merged in: sage-4.7.alpha4
Authors: John Palmieri Reviewers: Rob Beezer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by rbeezer)

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

Attachments (2)

trac_4983-subdivisions.patch (24.8 KB) - added by jhpalmieri 8 years ago.
trac_4983-subdivisions-rebased.patch (26.2 KB) - added by jhpalmieri 8 years ago.

Download all attachments as: .zip

Change History (21)

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

  • Type changed from defect to enhancement

comment:3 Changed 9 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 9 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 8 years ago by jason

This probably conflicts with #10847 too.

comment:6 Changed 8 years ago by jason

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

comment:7 Changed 8 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: Changed 8 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 8 years ago by kcrisman

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.

Changed 8 years ago by jhpalmieri

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

  • Status changed from positive_review to needs_work

This patch conflicts with #10974.

comment:12 follow-up: Changed 8 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?

Changed 8 years ago by jhpalmieri

comment:13 Changed 8 years ago by jhpalmieri

  • Description modified (diff)

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

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

comment:16 follow-up: Changed 8 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 8 years ago by jdemeyer

  • Status changed from needs_review to positive_review

Replying to kcrisman:

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

Done.

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

  • Description modified (diff)

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

comment:19 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.7.alpha4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.