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:

Status badges

Description (last modified by rbeezer)

This will vastly improve the documentation of the random_matrix command in common use cases (integers, rationals,...).

It will also allow for new, more specialized constructions, such as Billy Wonderly's work at #9720, #9754, #9802.

See #9720 for motivating discussion.

Attachments (2)

trac_9803-random-matrix-constructor-v1.patch (29.3 KB) - added by rbeezer 11 years ago.
trac_9803-random-matrix-constructor-v2.patch (29.3 KB) - added by rbeezer 11 years ago.
Standalone patch

Download all attachments as: .zip

Change History (12)

comment:1 Changed 11 years ago by rbeezer

  • Authors set to Rob Beezer
  • Cc mhansen wdj jason added
  • Description modified (diff)
  • Status changed from new to needs_review

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 new random_*_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 the random_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 in None to make this happen. It was not documented.

        if density is None:
            A.randomize(density=float(1), nonzero=False, *args, **kwds)
        else:
            A.randomize(density=density, nonzero=True, *args, **kwds)

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.

Changed 11 years ago by rbeezer

Standalone patch

comment:2 Changed 11 years ago by rbeezer

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: Changed 11 years ago by wdj

Does this patch depend on any other patches?

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

Replying to wdj:

Does this patch depend on any other patches?

Ah, yes, totally forgot. It needs to have #9720 (just the v4 patch) applied first. Thanks.

comment:5 Changed 11 years ago by wdj

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: Changed 11 years ago by mhansen

This looks good to me.

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

  • 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 rbeezer

  • Reviewers set to David Joyner, Mike Hansen

comment:9 Changed 11 years ago by bwonderly

Release Manager

#9720, #9803, #9802, #9754 is each dependent on the predecessor, merge in this order.

comment:10 Changed 11 years ago by mpatel

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