GenericGraph.[weighted]adjacency_matrix, incidence_matrix: Accept keyword arguments for matrix constructor
Closed this issue · 22 comments
Depends on #32465
CC: @dcoudert
Component: graph theory
Author: Matthias Koeppe
Branch/Commit: 0bd2930
Reviewer: David Coudert
Issue created by migration from https://trac.sagemath.org/ticket/33377
Author: Matthias Koeppe
Last 10 new commits:
2ffb118 | Vector_double_dense: Factor through new class Vector_numpy_dense |
5b288c3 | sage.modules.vector_numpy_integer_dense: New |
c8eec7e | Merge tag '9.5.beta6' into t/32465/refactor_matrix_double_dense_through_matrix_numpy_dense |
5323b25 | src/sage/matrix/matrix_numpy_dense.pyx: Returns -> Return |
d07cf93 | Merge tag '9.5' into t/32465/refactor_matrix_double_dense_through_matrix_numpy_dense |
93f3925 | src/sage/matrix/matrix_numpy_dense.pyx: Fix doc markup, whitespace |
21ed44a | Merge tag '9.6.beta1' into t/32465/refactor_matrix_double_dense_through_matrix_numpy_dense |
fe1da0a | Merge #32465 |
007253d | src/sage/matrix/matrix_space.py (get_matrix_class): Handle base_ring=ZZ, implementation='numpy' |
01d5541 | GenericGraph.adjacency_matrix: Accept keyword arguments for matrix constructor |
I'm OK with this improvement, but the same should be done for weighted_adjacency_matrix.
These methods could benefit further improvements. So far we first feed a dictionary and then build the matrix. It might be faster to directly feed the matrix.
Reviewer: David Coudert
Thanks for the feedback! Here's the corresponding change for weighted_adjacency_matrix.
Replying to @dcoudert:
These methods could benefit further improvements. So far we first feed a dictionary and then build the matrix. It might be faster to directly feed the matrix.
Yes, but I won't work on it in this ticket
Branch pushed to git repo; I updated commit sha1. New commits:
e30c7cb | GenericGraph.weighted_adjacency_matrix: Make base_ring a keyword-only argument |
Branch pushed to git repo; I updated commit sha1. New commits:
6f2185a | GenericGraph.incidence_matrix: Accept keyword arguments for matrix constructor |
Branch pushed to git repo; I updated commit sha1. New commits:
0bd2930 | GenericGraph._matrix_: Use the new keyword argument base_ring of GenericGraph.adjacency_matrix |
I'll stop here, but the same could be done to BipartiteGraph.reduced_adjacency_matrix and some other methods that I see in git grep ' def.*_matrix' src/sage/graphs/
For distance_matrix, kirchhoff_matrix it is trickier because these methods already accept **kwds that they pass on to some other method.
LGTM.
I will check the other cases.
For kirchhoff_matrix, all arguments in **kwds are passed to adjacency_matrix or weighted_adjacency_matrix. So it is now possible to specify the base ring.
A question is whether we should allow passing arguments for the matrix constructor to methods calling adjacency_matrix and similar methods.
Replying to @dcoudert:
For
kirchhoff_matrix, all arguments in**kwdsare passed toadjacency_matrixorweighted_adjacency_matrix. So it is now possible to specify the base ring.
Thanks for checking! If immutable=True is passed, the result matrix will have to be set_immutable. Could be done in #33388
Thanks for reviewing!
Changed branch from u/mkoeppe/use_numpy_integer_dense_for_adjacency_matrices to 0bd2930