Add Arbitrary implementation for other types in fp
#174
Conversation
ext/crates/fp/src/matrix/subspace.rs
Outdated
| Matrix::arbitrary_with(MatrixArbParams { | ||
| p: Some(p), | ||
| rows: (0..=dim + 1).boxed(), | ||
| columns: Just(dim).boxed(), | ||
| }) |
There was a problem hiding this comment.
Is this likely to generate interesting subspaces? They will all be of full or nearly full rank. I'd think you should first pick the rank k, generate an arbitrary rank k * dim matrix, and then for each additional row pick a random linear combination of the first k rows. The last row can be left as all 0. Alternatively, it might be better to first pick a random row echelon form and then randomly pick the entries in the non-pivot columns.
There was a problem hiding this comment.
The matrices themselves will be full rank the majority of the time, but they'll define a subspace of an ambient space of fixed dimension dim. The dimension of the subspace itself is the rank of the matrix, but because the number of rows ranges over 0..=dim+1, I think we hit every subspace that way.
Is your point that we should test creating subspaces with highly rank deficient matrices? One of the invariants of Subspace is that it always contains a row-reduced matrix, so it'll be the same as creating a subspace with a smaller matrix and extending with rows of 0s. We could add a test that creates those deficient matrices and checks that the matrix does become row-reduced
There was a problem hiding this comment.
Oh I see, so columns is dim but rows is anywhere from 0 to dim + 1 so you will get low dimensions. I'm not so concerned about testing the row reduction algorithm itself, if we want low rank matrices that are not already mostly zeros we can generate them specifically for row reduction tests.
I'm still a bit concerned that practically all the matrices will be chosen in the open Schubert cell (where all the pivots lie along the main diagonal)... but perhaps there aren't too many tests where that matters. What would you say about randomly choosing a subset of columns to get pivots, then place the pivots, and randomly fill the entries in the non-pivot columns. That would create subspaces evenly distributed across all the Schubert cells of all the grassmannians.
There was a problem hiding this comment.
That's good at least, but won't we still almost always end up in the dense Schubert cell for Gr_{k, dim} where the pivots lie along the main diagonal? I'd think what we'd want is to pick a random subset of k of the dim columns to be the pivot columns, and then fill the non pivot columns with random data.
There was a problem hiding this comment.
I simulated that by building an arbitrary matrix and forcing some pivots by filling initial segments with 0s. That should give more interesting subspaces
| let sub = Subspace::arbitrary_with(SubspaceArbParams { | ||
| p: Some(p), | ||
| dim: Just(dim).boxed(), | ||
| }); | ||
| let quotient = Subspace::arbitrary_with(SubspaceArbParams { | ||
| p: Some(p), | ||
| dim: Just(dim).boxed(), | ||
| }); |
There was a problem hiding this comment.
Doesn't the quotient have to be a sub of the sub here?
There was a problem hiding this comment.
It would be helpful to update this file with a comment explaining the invariants that sub and quotient passed in are supposed to satisfy and what additional invariants are satisfied when in normal form.
There was a problem hiding this comment.
I agree it's weird that quotient doesn't have to define a subspace of sub, but that's because a Subquotient containing Subspaces gens and quotient really represents the space (gens + quotient) / quotient, mathematically speaking.
This is documented at Subquotient::from_parts, but maybe it should be somewhere else too?
There was a problem hiding this comment.
How about at the top of the file?
There was a problem hiding this comment.
Good idea, I'll move it there and add a link to it at Subquotient::from_parts
JoeyBF
force-pushed
the
fix_subquotient
branch
from
45491c4 to
00d649e
Compare
|
Thanks! I added a few changes to export the new modules and update the integration test. I never realized we were only testing row-reduced matrices with 0s in the non-pivot columns, but now we're testing them all! |
* Implement `Arbitrary` for `FqVector` * Implement `Arbitrary` for `Matrix` * Implement `Arbitrary` for `Subspace` * Implement `Arbitrary` for `Subquotient` * Add proptest for `Subquotient` * Fix bug in `Subquotient::complement_pivots` * Introduce `Matrix::arbitrary_rref` * Export `arbitrary` modules properly * Use `Matrix::arbitrary_rref` for `row_reduce` integration test
…nces#174) * Implement `Arbitrary` for `FqVector` * Implement `Arbitrary` for `Matrix` * Implement `Arbitrary` for `Subspace` * Implement `Arbitrary` for `Subquotient` * Add proptest for `Subquotient` * Fix bug in `Subquotient::complement_pivots` * Introduce `Matrix::arbitrary_rref` * Export `arbitrary` modules properly * Use `Matrix::arbitrary_rref` for `row_reduce` integration test
I noticed a bug in
Subquotient::complement_pivots, so I wanted to add a test to cover that. That led me to implement proptest'sArbitraryfor several types. This PR also fixes the bug