Rework sympy to pymbolic mappers by alexfikl · Pull Request #305 · inducer/sumpy

alexfikl · 2026-09-04T18:26:22Z

The mappers previously inherited from either the Sympy or SymEngine variants based on USE_SYMENGINE. This meant that you couldn't use them at all for the other one, e.g. if USE_SYMENGINE=True there was no way to convert sympy expressions.

This implements both variants (with some code duplication) and adds to_pymbolic and to_symbolic helper functions that do the actual dispatch based on USE_SYMENGINE.

alexfikl · 2026-09-04T18:30:53Z

Hmm.. this needs a bit more work. It needs to make a distinction between USE_SYMENGINE and HAS_SYMENGINE..

alexfikl

Worth noting: this is pretty breaking because PymbolicToSympy just does sympy now. From what I can tell, these are undocumented classes that are not used by pytential or anyone.. so not a big deal? 😁

alexfikl · 2026-09-05T08:37:17Z

+    symbols: bool
+
+    def __init__(self, *, symbols: bool = False) -> None:
+        self.symbols = symbols


This also adds a little attribute to the mapper instead of having another mapper altogether. Good idea? Seemed cleaner..

It's not well-explained. I have a vague sense of what it does, but that's based on Ctrl+F. Add a docstring?

Added some docs in https://github.com/inducer/sumpy/compare/f4c6f95906def291811757bc6aa8e54fb6d09fff..eee22cd78a2bec98513172b412185a449811bd97.

alexfikl · 2026-09-05T08:39:10Z

+        if not self.symbols:
+            return super().map_ImaginaryUnit(expr)
+
+        return prim.Variable("I")


I don't think this is used anywhere (will be in #279), but it didn't roundtrip I correctly at all before.

Shouldn't this just become 1j?

I'm not sure? This tries to roundtrip with SympyToPymbolic mostly + places like HelmholtzKernel use var("I")..

Copilot

🟡 Changes recommended

The SymEngine symbols=True conversion path does not currently map the imaginary unit I as expected, which can break the new roundtrip behavior covered by tests.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR decouples symbolic↔pymbolic conversion from the active backend (USE_SYMENGINE) by implementing explicit sympy and SymEngine mapper variants and introducing to_pymbolic / to_symbolic (and to_symbolic_expr) helpers that dispatch appropriately. This allows converting expressions for either backend even when the other backend is selected at runtime.

Changes:

Add to_pymbolic, to_symbolic, and to_symbolic_expr helpers and refactor mapper implementations in sumpy/symbolic.py.
Refactor call sites across the codebase to use the new helper functions instead of directly instantiating backend-specific mappers.
Extend roundtrip tests to cover additional known symbols (notably the imaginary unit I).

File summaries

File	Description
sumpy/tools.py	Switch kernel scaling constant conversion to `sym.to_pymbolic`.
sumpy/test/test_misc.py	Update symbolic roundtrip test to use helpers and include `I`.
sumpy/test/test_codegen.py	Use `to_pymbolic` helper for sym→pymbolic conversion in codegen test.
sumpy/symbolic.py	Introduce helper dispatch + implement both sympy and SymEngine mapper variants.
sumpy/qbx.py	Use `sym.to_symbolic_expr` for strengths conversion.
sumpy/kernel.py	Use `sym.to_symbolic_expr(..., symbols=True)` for expression/scaling conversion.
sumpy/e2p.py	Use `to_pymbolic` helper for kernel scaling conversion.
sumpy/codegen.py	Remove local sym→pymbolic mapper and rely on `sym.to_pymbolic`.
.basedpyright/baseline.json	Update pyright baseline to reflect new/changed typing diagnostics.

Review details

Files reviewed: 9/9 changed files
Comments generated: 1
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

inducer

Thanks! A few comments/questions below.

inducer · 2026-09-05T18:58:32Z

+    symbols: bool
+
+    def __init__(self, *, symbols: bool = False) -> None:
+        self.symbols = symbols


It's not well-explained. I have a vague sense of what it does, but that's based on Ctrl+F. Add a docstring?

inducer · 2026-09-05T18:58:47Z

+    symbols: bool
+
+    def __init__(self, *, symbols: bool = False) -> None:
+        self.symbols = symbols


Shouldn't it be declared in the mixin instead?

Sure. I didn't put it in there because in my head mixins don't carry state, but I guess it doesn't matter that much. pyright is also very upset by the mixin 😢

Well, if methods in the mixin rely on a bit of data, the same class should declare what it's supposed to be. (IMO)

Moved in https://github.com/inducer/sumpy/compare/f4c6f95906def291811757bc6aa8e54fb6d09fff..eee22cd78a2bec98513172b412185a449811bd97.

inducer · 2026-09-05T18:59:16Z

+    symbols: bool
+
+    def __init__(self, *, symbols: bool = False) -> None:
+        self.symbols = symbols


Is symbols=False ever needed really?

I think at least some of the codegen needs it so that things like var("pi") from the kernels get transformed into numbers (which is what happens with symbols=False). Not sure this is the best place to do that..

inducer · 2026-09-05T18:59:44Z

+        if not self.symbols:
+            return super().map_ImaginaryUnit(expr)
+
+        return prim.Variable("I")


Shouldn't this just become 1j?

alexfikl marked this pull request as draft September 4, 2026 18:26

alexfikl force-pushed the refactor-sympy-mapper branch 3 times, most recently from ebf1188 to 3d633e9 Compare September 5, 2026 08:33

alexfikl commented Sep 5, 2026

View reviewed changes

alexfikl marked this pull request as ready for review September 5, 2026 08:41

alexfikl requested a lite review from Copilot September 5, 2026 08:41

Copilot started reviewing on behalf of alexfikl September 5, 2026 08:41 View session

Copilot AI reviewed Sep 5, 2026

View reviewed changes

Comment thread sumpy/symbolic.py

alexfikl force-pushed the refactor-sympy-mapper branch from 3d633e9 to f4c6f95 Compare September 5, 2026 11:17

inducer reviewed Sep 5, 2026

View reviewed changes

alexfikl force-pushed the refactor-sympy-mapper branch from f4c6f95 to eee22cd Compare September 6, 2026 07:16

alexfikl added 4 commits September 6, 2026 11:29

feat: rework sympy to pymbolic mappers

70e0a87

fix: roundtrip imaginary I to sympy

2354ea2

chore: update baseline

3965ebf

docs: explain symbol handling in sympy-pymbolic mappers

e787c4d

alexfikl force-pushed the refactor-sympy-mapper branch from eee22cd to e787c4d Compare September 6, 2026 08:29

Conversation

alexfikl commented Sep 4, 2026

Uh oh!

alexfikl commented Sep 4, 2026

Uh oh!

alexfikl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexfikl Sep 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

inducer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alexfikl Sep 5, 2026 •

edited

Loading