-
-
Notifications
You must be signed in to change notification settings - Fork 652
Add functions for computation of krylov basis and krylov kernel basis #40508
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
…dn_dense, add aliases for pivots and pivot_rows, optimisation for krylov_rank_profile by minimising index conversion and full permutation calculation
…ial creation in basis calculation
Some benchmarks are provided here (done in a branch with #40423 and #40435 merged):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments on the doctests to start with.
In terms of efficiency, since this has been dropped (it should be if it is not correct, thanks for checking carefully), I assume this means that an additional RREF is computed on the transpose? LinBox/FFLAS-FFPACK does incorporate echelonization procedures which preserve the row rank profile as well as the column rank profile. We could try to include this in Sage. This would avoid wasting a factor of 2 by calling the echelonization twice when both are needed. Would you mind opening an issue for this? (and you could link to your above message or copy it there, to show the observation that a trivial deduction from P does not always work) |
I've made many of the changes in comments, but realised my original naive implementation during testing had performance problems and with the necessary fixes should be the implementation for smaller cases. I'm currently working on mapping the parameter criteria as the relation is not trivial. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some additional comments; however, my review is not finished (I will continue later).
if M.base_ring() != E.base_ring(): | ||
E, M = coercion_model.canonical_coercion(E, M) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we want to change the base ring of self
.
I think I would prefer:
M = M.change_ring(E.base_ring())
but I'm open to the discussion if you think that it is not a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we happy that in the case E
has a smaller base ring than M
, for example E
is over GF(7) and M
is over GF(49) that the operation should fail (given M
actually has elements not in GF(7))? Currently, E
would be coerced to GF(49) in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
I think you cannot use it here because you have extra arguments but let me mention the decorator @coerce_binop
which basically does what you are doing.
src/sage/matrix/matrix2.pyx
Outdated
- ``output_rows`` -- determines whether the output row indices | ||
are pairs of row indices in ``self`` and degrees of ``M`` (False) or | ||
row indices in the Krylov matrix (True). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should update these lines, I think.
src/sage/matrix/matrix2.pyx
Outdated
- ``output_rows`` -- determines whether the output row indices | ||
are pairs of row indices in ``self`` and degrees of ``M`` (False) or | ||
row indices in the Krylov matrix (True). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same remark as above.
src/sage/matrix/matrix2.pyx
Outdated
- the rows of ``C`` form a minimal basis for the kernel of | ||
``self.striped_krylov_matrix(M,P.degree())`` | ||
|
||
TESTS:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of them should be EXAMPLES
.
I have one uncertainty about what expected behaviour should be. Should the degree bounds impact the returned row indices that refer to the Krylov matrix? Currently placing different degree bounds for each row changes the returned row indices for certain inputs, such as in the following:
This is because the naive method computes a krylov matrix only up to the degrees specified. (The elimination method is adjusted to return degrees consistent with this.)
It doesn't sit completely right with me, having an argument that is provided for performance, affect the value returned. I was considering adjusting the returned row indices. However, even choosing a uniform bound causes inconsistencies:
An alternative solution would be to give the indices as they would appear in an "infinite" Krylov matrix, i.e. where all degrees of M appear. This is inconsistent with the examples given in https://arxiv.org/abs/1512.03503 however. |
IMO, the outputted index row should be the index of the row as it appears in the output of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some further comments.
src/sage/matrix/matrix2.pyx
Outdated
if degree is None: | ||
degree = vector(ZZ, [E.ncols()] * E.nrows()) | ||
elif isinstance(degree, (FreeModuleElement, list, tuple)): | ||
degree = (ZZ**E.nrows())(degree) | ||
else: | ||
degree = (ZZ**E.nrows())([degree] * E.nrows()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it important that degree
is a vector and not just a list?
By the way, don't we need to use the plural for degrees
(similarly to shifts
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main advantage to this is the automatic type conversion. If degree
contains 1.0
or 10/5
then ZZ**E.nrows()
will automatically check if conversion is possible. A secondary advantage is that this automatically checks that the length of the list, tuple, or vector is correct.
I'll change degree
to degrees
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed that there exists a method _check_shift_dimension
.
Probably, it is better to use it for shifts
and have a similar methods for degrees
(it will avoid code duplication). This method currently does not check that the entries are integers but maybe we can add it.
src/sage/matrix/matrix2.pyx
Outdated
- ``degree`` -- upper bound on degree of minpoly(`M`). If None, a | ||
suitable upper bound of ``self.ncols()`` is default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should say somewhere that if the bound is not correct, then the output may be incorrect as well.
for row in range(m): | ||
coeffs_map[row][c[col]][d[col]] = coeffs_map[row][c[col]].get(d[col], base_ring.zero()) - relation[row,col] | ||
|
||
# convert to matrix (slow for extension fields) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably should be fixed in the __init__
/_element_constructor_
method of the relevant class.
But that's for another PR.
Yes, I agree with this. @Biffo89 can you confirm this is the behaviour currently? (it seems to be, from your examples above) If yes, then I would not change anything regarding this point.
We do not have to see this as some inconsistency, and we do not have to see the degree bounds as being here for performance only. To expand on xcaruso's comment: the degree bounds w.r.t each row of |
# INPUT VALIDATION | ||
if not isinstance(shifts, FreeModuleElement) or shifts not in ZZ**self.nrows(): | ||
raise TypeError('_krylov_row_coordinates: shifts must be an integer vector of length sel.nrows().') | ||
if not isinstance(degrees, FreeModuleElement) or degrees not in ZZ**self.nrows(): | ||
raise TypeError('_krylov_row_coordinates: degrees must be an integer vector of length sel.nrows().') | ||
if row_pairs is not None and (not isinstance(row_pairs, (list, tuple)) or any([not isinstance(x, tuple) or len(x) != 2 or any([not isinstance(y, (int, sage.rings.integer.Integer)) for y in x]) for x in row_pairs])): | ||
raise TypeError('_krylov_row_coordinates: row_pairs must be a list or tuple of tuple pairs of integers.') | ||
if any([d < 0 for d in degrees]): | ||
raise ValueError('_krylov_row_coordinates: degrees cannot have negative entries.') | ||
if row_pairs is not None and any([0 > r[0] or r[0] >= self.nrows() for r in row_pairs]): | ||
raise ValueError('_krylov_row_coordinates: row_pairs cannot have out of bounds rows.') | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I think that those lines are not needed for a private method.
if M.base_ring() != E.base_ring(): | ||
E, M = coercion_model.canonical_coercion(E, M) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
I think you cannot use it here because you have extra arguments but let me mention the decorator @coerce_binop
which basically does what you are doing.
row_coordinates = self._krylov_row_coordinates(shifts, degrees) | ||
|
||
row_profile = tuple([(*row_coordinates[i][:2], i) for i in row_profile]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between row_coordinates
and row_profile
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
row_coordinates[_][:2]
is a sorted list of the row-degree pairs (c,d)
in K
. row_profile
is initially the independent rows in K
. Line 19523 uses row_coordinates
to update row_profile
to add the corresponding row in self
(row_coordinates[_][0]
) and degree of M
(row_coordinates[_][1]
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, my comment was very very badly formulated.
I was in fact wondering if you cannot simply write
row_profile = tuple([row_coordinates[i] for i in row_profile])
else: | ||
M_L = M_L * M_L | ||
|
||
R = matrix.block([[exhausted],[R],[(R*M_L).matrix_from_rows([i for i in range(len(row_profile_self)) if row_profile_self[i][1] + L <= degrees[row_profile_self[i][0]]])]], subdivide=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is very long. For readibility, I suggest to cut it:
R = matrix.block([[exhausted],[R],[(R*M_L).matrix_from_rows([i for i in range(len(row_profile_self)) if row_profile_self[i][1] + L <= degrees[row_profile_self[i][0]]])]], subdivide=False) | |
rows = [i for i, x in enumerate(row_profile_self) if x[1] + L <= degrees[x[0]]] | |
S = (R*M_L).matrix_from_rows(rows) | |
R = matrix.block([[exhausted],[R],[S]], subdivide=False) |
k_coordinates = self._krylov_row_coordinates(shifts, degrees, k) | ||
k_perm = Permutation([x[2] + 1 for x in k_coordinates]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
k_coordinates = self._krylov_row_coordinates(shifts, degrees, k) | |
k_perm = Permutation([x[2] + 1 for x in k_coordinates]) | |
k_rows = [x[2] for x in self._krylov_row_coordinates(shifts, degrees, k)] | |
k_perm = Permutation([x + 1 for x in k_rows]) |
so that, instead of writing k_perm(i+1)-1
everywhere afterwards, we can just write k_rows[i]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe _krylov_row_coordinates
could have an option for just returning the row index.
|
||
# INPUT VALIDATION | ||
if not (E.base_ring() in _Fields and E.base_ring().is_exact()): | ||
raise TypeError("krylov_basis: matrix entries must come from an exact field") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TypeError
or NotImplementedError
?
This PR implements the following functions, adds aliases
row_rank_profile
forpivot_rows
andcolumn_rank_profile
forpivots
, and caches the row rank profile that is calculated as part oflinbox_echelonize
inmatrix_modn_dense_template.pxi
.Given a$\mathbb{K}[x]$ -module $V$ with multiplication matrix $J \in \mathbb{K}^{\sigma\times\sigma}$ , a set of $m$ elements of $V$ expressed in matrix form $E \in \mathbb{K}^{m\times\sigma}$ , a priority shift $s = (s_1,\dots,s_m) \in \mathbb{Z}^m$ , and an upper bound $d$ on the degree of the minimal polynomial of $J$ , $P(E_{c,*} J^d) = s_c + d$ :
krylov_rank_profile
finds the lexicographical first maximal linearly independent set of rows from the matrix, with rows ordered according to the priorityIt returns the positions of these rows in the permuted matrix, the submatrix formed by these rows, and the lexicographical first maximal linearly independent set of columns of the submatrix.
Given the same arguments and a variable name,$s$ -minimal interpolation basis for $(E,J)$ , i.e. an $s$ -reduced basis for the space of polynomials satisfying $p_1 E_{1,*}+\dots+p_mE_{m,*}=0$ .
linear_interpolation_basis
calculates an📝 Checklist