Cyclic subspaces (aka Krylov subspaces)

[rbeezer]

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

Apply:

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

Component: linear algebra

Author: Rob Beezer

Reviewer: Timo Kluck

Merged: sage-5.11.beta0

Issue created by migration from https://trac.sagemath.org/ticket/11364

[rbeezer]

comment:1

Attachment: trac_11364-cyclic-subspaces.patch.gz

[rbeezer]

Author: Rob Beezer

[tkluck]

comment:3

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.

[tkluck]

Attachment: trac_11364_review_comments.patch.gz

[rbeezer]

comment:4

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

[rbeezer]

Reviewer: Timo Kluck

[rbeezer]

comment:5

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

[jdemeyer]

comment:6

Never use

except:

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

except Exception:

if you want a catch-all.

[rbeezer]

comment:8

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

[rbeezer]

comment:9

Attachment: trac_11364_review_comments-v2.patch.gz

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

[tkluck]

comment:10

Replying to @rbeezer:

Timo - can you bless the changes?

Definitely.

[jdemeyer]

Merged: sage-5.11.beta0