Opened 10 years ago

Closed 8 years ago

#11364 closed enhancement (fixed)

Cyclic subspaces (aka Krylov subspaces)

Reported by: rbeezer 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 rbeezer)

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 rbeezer 10 years ago.
trac_11364_review_comments.patch (1.9 KB) - added by tkluck 8 years ago.
trac_11364_review_comments-v2.patch (1.9 KB) - added by rbeezer 8 years ago.

Download all attachments as: .zip

Change History (15)

Changed 10 years ago by rbeezer

comment:1 Changed 10 years ago by rbeezer

  • Authors set to Rob Beezer
  • Description modified (diff)

comment:2 Changed 10 years ago by rbeezer

  • Status changed from new to needs_review

comment:3 follow-up: Changed 8 years ago by tkluck

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 8 years ago by tkluck

comment:4 in reply to: ↑ 3 Changed 8 years ago by rbeezer

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 8 years ago by rbeezer

  • Description modified (diff)
  • Reviewers set to Timo Kluck
  • Status changed from needs_review to positive_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 follow-up: Changed 8 years ago by jdemeyer

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 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

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

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 8 years ago by rbeezer

comment:9 follow-up: Changed 8 years ago by rbeezer

  • Description modified (diff)
  • Status changed from needs_work to needs_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 8 years ago by tkluck

  • Status changed from needs_review to positive_review

Replying to rbeezer:

Timo - can you bless the changes?

Definitely.

comment:11 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.10 to sage-5.11

comment:12 Changed 8 years ago by jdemeyer

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