Skip to content

Reorganize linear algebra #2037

Open
jessegrabowski wants to merge 30 commits intov3from
linalg-reorg
Open

Reorganize linear algebra #2037
jessegrabowski wants to merge 30 commits intov3from
linalg-reorg

Conversation

@jessegrabowski
Copy link
Copy Markdown
Member

v3 is a good excuse to bite this bullet. The current organization is based on which functions come from scipy/numpy, which is arbitrary and unclear. This PR reorganizes everything based on what the Ops do:

  • All linalg Ops live in tensor/_linalg
  • Inside linlag, we sort Ops by their function: solve, decomposition, summary, invert, etc.
  • Within the larger groups (solve, decomposition), we further decompose: solve.general, solve.psd, decoposition.svd, decomposition.lu, etc

This sorting is canonical and used consistently. All other modules that care about linalg ops (linker dispatches, rewrites, tests) follow the same organization.

@jessegrabowski
Copy link
Copy Markdown
Member Author

I forgot to mention:

  • Everything is re-imported into tensor/linalg.py. This is the entry point for the public API
  • I chose to only expose the helper functions rather than the Ops themselves. Internally, we should import from the true location.
  • nlinalg and slinalg can be imported from, but emit a warning.

The scan-tracking solve rewrites previously lived in
pytensor/tensor/_linalg/solve/rewriting.py and were registered via a
side-effect import chain from tensor/_linalg/__init__.py. That chain
was broken in the linalg reorg (the _linalg package init was emptied),
and registration only kept working incidentally because
tensor/linalg.py happens to import from _linalg.solve.

Move them into tensor/rewriting/linalg/solve.py alongside the other
solve rewrites. The imports also needed updating: the v3 file imported
via tensor.slinalg, which is now the deprecation shim and would emit
DeprecationWarnings on every load.

Also drop the speculative __all__ and inv_to_solve re-export from
tensor/rewriting/linalg/__init__.py; the one test that imported
inv_to_solve from there now imports it from .inverse directly.
These three test files happened to import individual Ops/functions from
pytensor.tensor.{nlinalg,slinalg}. Since those modules are now
deprecation shims, every test run was emitting DeprecationWarnings for
imports that have nothing to do with testing the shim itself.

Retarget the imports to the new pytensor.tensor._linalg.* locations,
matching the convention used by the new tests/tensor/linalg/ tests.
tests/tensor/test_nlinalg.py and tests/tensor/test_slinalg.py were left
untouched by the linalg reorg and still imported individual symbols
from pytensor.tensor.{nlinalg,slinalg}. With those modules now being
deprecation shims, every collection of these files emitted dozens of
DeprecationWarnings.

These files are not testing the deprecation shim itself — they're the
original Op test suites. Retarget the imports to the new
pytensor.tensor._linalg.* locations.
Asserts that accessing a moved name on the deprecated module emits a
DeprecationWarning, that the shim forwards to the same object exposed
by the new pytensor.tensor.linalg public API, and that unknown names
still raise AttributeError. Also fixes a stale doc comment in
_linalg/summary.py that pointed at the old nlinalg path.
The deprecation pages still carried `.. automodule:: pytensor.tensor.{nlinalg,slinalg} :members:` directives. Each automodule
walks the module's `__dir__`, which on the new shims returns every
moved name and triggers `__getattr__` per name — emitting a
DeprecationWarning per attribute on every doc build, and producing
duplicate doc entries that already exist on the new
:ref:`libdoc_linalg` page.

Drop the automodule blocks; the pages are now pure deprecation notices
that point at `pytensor.tensor.linalg`.
The linalg.rst page used `.. automodule:: pytensor.tensor.linalg
:members:`, which renders all 36 functions from `__all__` as one
unstructured alphabetical list. Replace it with hand-grouped sections
(Constructors / Decomposition / Inverse / Products / Solve / Summary)
that mirror the internal `_linalg/` package layout, using explicit
`autofunction::` directives.

To stop the rst from drifting away from the public API, add
`tests/tensor/linalg/test_doc_api.py`. It parses every
`.. autofunction:: pytensor.tensor.linalg.<name>` directive out of the
rst and asserts the resulting set is exactly equal to
`pytensor.tensor.linalg.__all__`. Any future addition or removal on
either side fails the test with a precise diff.

Op classes remain undocumented in the rst — the public surface is
intentionally functions only.
@ricardoV94
Copy link
Copy Markdown
Member

Cleaned up a bit more (including docs), and internal references that were triggering the new DeprecationWarning. We still have tests/tensor/test_slinalg / nlinalg. We can leave those for later. I added a short test for the DeprecatedShim to make sure it works. It's your round to review now

@jessegrabowski
Copy link
Copy Markdown
Member Author

You changes all look good.

My last doubt is about the name of the private module. _linalg is a bit unsightly, and it's also not truely private. There are times we expect users to use the actual linalg Ops, which would require going in there. So it sort of breaks the underscore contract. We're not currently exposing them (only the instances/functions) for cleanliness in the namespace.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants