Skip to content

Defer optional SymPy imports until symbolic operations need them - #264

Open
RoboX2020 wants to merge 7 commits into
rai-opensource:masterfrom
RoboX2020:perf/lazy-sympy
Open

RoboX2020 wants to merge 7 commits into
rai-opensource:masterfrom
RoboX2020:perf/lazy-sympy

Conversation

@RoboX2020

Copy link
Copy Markdown

Part of #234, covering the SymPy half only. The SciPy work remains separate in #244.

import spatialmath no longer loads SymPy just to check scalar types. issymbol() checks the already-loaded module, and scalar/vector checks use it dynamically so importing SymPy after SpatialMath works too.

The other eager SymPy imports in vector norms, object-matrix determinants and 2D pose extraction are deferred. Explicit symbolic helpers load the optional dependency when called; explicit access to Symbol and symtype remains available.

Added nine isolated-process test cases covering numeric imports and operations, both import orders, symbolic arithmetic and transforms, explicit helpers and legacy aliases, and numeric use with SymPy blocked.

Validation: Python 3.10/Linux, MPLBACKEND=Agg python -m pytest tests/ -p no:xvfb -q: 389 passed, 3 skipped, 18 subtests passed. No timing claim is made here. Other Python versions and platforms were not tested.

@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 75.43860% with 14 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
spatialmath/base/symbolic.py 80.00% 7 Missing ⚠️
spatialmath/base/transforms2d.py 16.66% 5 Missing ⚠️
spatialmath/base/transformsNd.py 71.42% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@petercorke petercorke left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Prajval, this is a well-scoped change. Checking sys.modules in issymbol() is a neat way to handle both import orders, and the isolated-subprocess tests, including the blocked-SymPy one, are exactly the right way to test this.

I merged it with current master locally: 419 passed, 3 skipped, import spatialmath no longer loads SymPy, and the isscalar/isvector timings are unchanged.

One thing to fix before merging:

  • pi() docstring in symbolic.py. The whole docstring has been indented by an extra 4 spaces, and a stray from typing import TYPE_CHECKING has landed inside the runblock example. That ends the block early, so the docs will render incorrectly, and CI doesn't catch it because the sphinx job is skipped. It looks like a find-and-replace that went astray; restoring the original docstring should fix it.

Two smaller things, neither blocking:

  • On master, from spatialmath.base.symbolic import * made sympy, Symbol and symtype available in the importing namespace (and as smb.sympy, smb.Symbol, smb.symtype) whenever SymPy was installed. With this PR those three names are no longer exported that way, because star-imports don't go through the module-level __getattr__. Explicit from spatialmath.base.symbolic import Symbol, symtype still works. I couldn't find any users in RTB, MVTB, bdsim or Swift, but it's worth a line in the description.
  • Codecov shows the ImportError fallbacks in tr2xyt and det aren't exercised. A test with an object-dtype array under the SymPy blocker would cover both.

Approving, please fix the docstring before merge.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants