Opened 11 years ago
Closed 9 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: |
Description (last modified by )
These are useful for topics related to rational canonical form (coming soon) and minimal polynomials.
Apply:
Attachments (3)
Change History (15)
Changed 11 years ago by
comment:1 Changed 11 years ago by
- Description modified (diff)
comment:2 Changed 11 years ago by
- Status changed from new to needs_review
comment:3 follow-up: ↓ 4 Changed 10 years ago by
Changed 10 years ago by
comment:4 in reply to: ↑ 3 Changed 10 years ago by
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 9 years ago by
- 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: ↓ 8 Changed 9 years ago by
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 9 years ago by
- Status changed from positive_review to needs_work
comment:8 in reply to: ↑ 6 Changed 9 years ago by
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 9 years ago by
comment:9 follow-up: ↓ 10 Changed 9 years ago by
- 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 9 years ago by
- Status changed from needs_review to positive_review
comment:11 Changed 9 years ago by
- Milestone changed from sage-5.10 to sage-5.11
comment:12 Changed 9 years ago by
- Merged in set to sage-5.11.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
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 functioncyclic_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.