Opened 12 years ago

Closed 10 years ago

#11364 closed enhancement (fixed)

Cyclic subspaces (aka Krylov subspaces)

Reported by: Rob Beezer Owned by: jason, was
Priority: minor Milestone: sage-5.11
Component: linear algebra Keywords:
Cc: Merged in: sage-5.11.beta0
Authors: Rob Beezer Reviewers: Timo Kluck
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Rob Beezer)

These are useful for topics related to rational canonical form (coming soon) and minimal polynomials.

Apply:

  1. trac_11364-cyclic-subspaces.patch
  2. trac_11364_review_comments-v2.patch

Attachments (3)

trac_11364-cyclic-subspaces.patch (15.9 KB) - added by Rob Beezer 12 years ago.
trac_11364_review_comments.patch (1.9 KB) - added by Timo Kluck 10 years ago.
trac_11364_review_comments-v2.patch (1.9 KB) - added by Rob Beezer 10 years ago.

Download all attachments as: .zip

Change History (15)

Changed 12 years ago by Rob Beezer

comment:1 Changed 12 years ago by Rob Beezer

Authors: Rob Beezer
Description: modified (diff)

comment:2 Changed 12 years ago by Rob Beezer

Status: newneeds_review

comment:3 Changed 10 years ago by Timo Kluck

This has been lying around on trac for a while; thought I could review it.

The code looks excellent and all tests pass. There seems to be a sufficient number of doctests. It was also a good decision to split the implementation into a "hidden" function _cyclic_subspace and the user-facing function cyclic_subspace with a friendlier interface.

The only remark I have is that I think that coercions could be handled a little better. For example, a call to cyclic_subspace now fails on matrices with integer coefficients, even though there exists a coercion ZZ to QQ. I patched it to try calling R.fraction_field().

I also moved the generator = False outside of the try block, because it is intended as a default value in case anything inside the try block fails.

If you agree with my changes, then by all means, mark this as a positive review.

Changed 10 years ago by Timo Kluck

comment:4 in reply to:  3 Changed 10 years ago by Rob Beezer

Replying to tkluck:

This has been lying around on trac for a while; thought I could review it.

Dear Timo,

Thank-you very much for looking at this. Yes, it is old, so I am surprised it has not bit-rotted in some way.

I will be offline for the next two weeks as part of the semester break here, so I'll have to take a close look later. But the changes in your patch certainly look like good improvements. I'll be back in a bit.

Thanks, Rob

comment:5 Changed 10 years ago by Rob Beezer

Description: modified (diff)
Reviewers: Timo Kluck
Status: needs_reviewpositive_review

Dear Timo,

Sorry to be so very long on this one - life got crazy there for a while. Your changes look very good to me. Thanks for the review and the great improvements.

As suggested above, I'll mark this "positive review" and we can finally get it in.

Thanks again, Rob

comment:6 Changed 10 years ago by Jeroen Demeyer

Never use

except:

because that can catch unwanted exceptions, like KeyboardInterrupt. Catch specific exceptions, or use

except Exception:

if you want a catch-all.

comment:7 Changed 10 years ago by Jeroen Demeyer

Status: positive_reviewneeds_work

comment:8 in reply to:  6 Changed 10 years ago by Rob Beezer

Replying to jdemeyer:

Catch specific exceptions

Right! I'm a little rusty. Thanks, Jeroen.

I'll likely fix this up on the reviewer patch.

Rob

Changed 10 years ago by Rob Beezer

comment:9 Changed 10 years ago by Rob Beezer

Description: modified (diff)
Status: needs_workneeds_review

Looks like a TypeError is the only possibility for the naked try. Thanks for catching that, Jeroen.

I made the change on the reviewer patch, which still has Timo's name on it (now named v2). I updated the summary line in the patch, since Trac numbers get prepended automatically now.

Timo - can you bless the changes?

Rob

comment:10 in reply to:  9 Changed 10 years ago by Timo Kluck

Status: needs_reviewpositive_review

Replying to rbeezer:

Timo - can you bless the changes?

Definitely.

comment:11 Changed 10 years ago by Jeroen Demeyer

Milestone: sage-5.10sage-5.11

comment:12 Changed 10 years ago by Jeroen Demeyer

Merged in: sage-5.11.beta0
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.