✨ sparse.csgraph: Refine laplacian return type#1688
Conversation
jorenham
changed the title
sparse.csgraph: Refine laplacian return type
|
jorenham
left a comment
jorenham
left a comment
There was a problem hiding this comment.
Thanks! The type alias changes make it a bit difficult for me to see what exactly changed here and what the consequences of these changes are. For that it would help if you could rebase onto main, because in the meantime type-tests have been added for sparse.csgraph.laplacian, which could help tell us if this leads to any regressions.
jorenham
left a comment
jorenham
left a comment
There was a problem hiding this comment.
The pending CI jobs are because of the new NumPy 2.5 release jobs I recently added, so if you could rebase then we can also test against that here.
The form="lo" case is now incorrect: it should return LinearOperator (with appropriate shape (so 2d I think) and dtype), but now it would return a plain Callable.
So the type test you added now incorrectly asserts that "form=lo" should return a callable, but that should be a LinearOperator.
| csgraph: _ToCSGraph, | ||
| normed: bool = False, | ||
| *, | ||
| return_diag: onp.ToTrue, |
There was a problem hiding this comment.
The return_diag param can also be used positioally, and since it's the third param, that's a pretty likely to be done in practice. But this changes it so that passing True for return_diag positionally will no longer be accepted; so that's a regression. The solution is to have another overload like this one that handles the positional return_diag case, i.e. where normed doesn't have a default.
| form: Literal["array"] = "array", | ||
| dtype: npt.DTypeLike | None = None, | ||
| symmetrized: bool = False, | ||
| ) -> _spbase: ... |
There was a problem hiding this comment.
Can we narrow this (and in the two overloads below)? returning a bare _spbase doesn't really rhyme with the intent of this PR.
|
|
||
| from scipy.sparse._base import _spbase | ||
| from scipy.sparse.linalg import LinearOperator | ||
| from scipy.sparse.linalg._interface import LinearOperator |
| type _LaplacianLO = LinearOperator[npc.number, tuple[int, int]] | ||
| type _ToCSGraph = onp.ToComplex2D | _spbase | ||
| type _Form = Literal["array", "lo"] | ||
| type _FunctionForm = Literal["function"] |
There was a problem hiding this comment.
Why does only "function" have a type alias, and not also the other "lo" and "array" literals? I don't care if you use type aliases or inline the Literals, as long as it's consistent.
| type _LaplacianFunction = Callable[[onp.ToComplex2D], onp.Array2D[npc.number]] | ||
| type _LaplacianMatrix = onp.Array2D[npc.number] | _spbase | LinearOperator | ||
| type _LaplacianDiag = onp.Array1D[npc.number] | ||
| type _LaplacianLO = LinearOperator[npc.number, tuple[int, int]] |
There was a problem hiding this comment.
The LinearOperator without generic arguments also accepts np.bool_, but now it doesn't anymore, so this narrows the dtype. Did you verify that at runtime np.bool_-values LinearOperators are indeed rejected? Because if there aren't (i.e. accepted) then this should also accept np.bool_.
| ) -> tuple[_LaplacianLO, _LaplacianDiag]: ... | ||
| @overload | ||
| def laplacian( # array form dense input -> dense output | ||
| csgraph: onp.ToComplex2D, |
There was a problem hiding this comment.
This broadens the accepted csgraph types in a type-unsafe way, because at runtime passing a nested sequence of float or something will crash.
| _fn1 = laplacian(_f64_nd, form="lo") | ||
| op.test.assert_subtype[LinearOperator[npc.number, tuple[int, int]]](_fn1) |
There was a problem hiding this comment.
This doesn't cover all overloads that you added.
| @overload | ||
| def laplacian( | ||
| csgraph: _ToCSGraph, | ||
| def laplacian( # array form dense input + return_diag -> dense output + diag |
There was a problem hiding this comment.
the comment here is the same as the one above, which tbh kinda defeats the purpose
2 participants
Towards #1445