Opened 11 years ago
Closed 11 years ago
#9803 closed enhancement (fixed)
Generalize and document the random_matrix constructor
Reported by: | rbeezer | Owned by: | jason, was |
---|---|---|---|
Priority: | major | Milestone: | sage-4.6 |
Component: | linear algebra | Keywords: | |
Cc: | bwonderly, mhansen, wdj, jason | Merged in: | sage-4.6.alpha1 |
Authors: | Rob Beezer | Reviewers: | David Joyner, Mike Hansen |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
Attachments (2)
Change History (12)
Changed 11 years ago by
comment:1 Changed 11 years ago by
- Cc mhansen wdj jason added
- Description modified (diff)
- Status changed from new to needs_review
comment:2 Changed 11 years ago by
First patch contained:
columns = parent.nrows()
which is just plain wrong, but also columns
was never referenced (which is why the doctests were unaffected). Its gone now. Use just the v2 patch.
comment:3 follow-up: ↓ 4 Changed 11 years ago by
Does this patch depend on any other patches?
comment:4 in reply to: ↑ 3 Changed 11 years ago by
comment:5 Changed 11 years ago by
Applied (with #9720) to 4.5.1 and passed sage -testall. Positive review as far as I see, but maybe Mike Hansen should weight in.
comment:6 follow-up: ↓ 7 Changed 11 years ago by
This looks good to me.
comment:7 in reply to: ↑ 6 Changed 11 years ago by
- Status changed from needs_review to positive_review
Replying to mhansen:
This looks good to me.
Thanks for your input. I think this is much improved organized this way, and I finally did something about the documentation for the random_matrix()
command. ;-)
Looks like this can go to positive review along with the rest of Billy's work.
comment:8 Changed 11 years ago by
- Reviewers set to David Joyner, Mike Hansen
comment:9 Changed 11 years ago by
comment:10 Changed 11 years ago by
- Merged in set to sage-4.6.alpha1
- Resolution set to fixed
- Status changed from positive_review to closed
Patch expands the functionality of the
random_matrix
routine. A matrix space is used to accumulate the base ring, dimensions and representation (sparse/dense). This can then be passed to the newrandom_*_matrix
routines where a matrix can actually be constructed and returned.Documentation for previous behavior greatly expanded, notably for integer and rational matrices. New routines are demonstrated, with clear directions (links, imports) to expanded documentation.
Had to handle density and sparse keywords in a backwards-compatible fashion, so they are "popped" out of the
kwds
dictionary and passed as before to the matrix randomize() routine. The keywords are now required and won't work as positional arguments. Had to adjust code in the group theory isomorphism code in a couple of modules as a result. Also therandom_matrix
command was employed coincidentally in a doctest in the lazy import routine. I think the new version works just as well as a test, so I changed the output.This code below looks like some artifact of the switch to allowing/disallowing zero entries. I've left it in, though it *never* gets called in any of the tests (I checked). Before my changes,
density
had a default value of 1, so you would have to consciously pass inNone
to make this happen. It was not documented.One fix in mod n dense matrix code. Could not figure out how
range(25)
was doing anything useful, and it was ending up in the algorithm argument, so in the end I just removed it and the affected test passes.