Skip to content

feat(dop): make Pdop and Sdop hashable for SymPy integration#546

Closed
utiberious wants to merge 1 commit intopygae:masterfrom
utiberious:fix/issue-234-pdop-sympy
Closed

feat(dop): make Pdop and Sdop hashable for SymPy integration#546
utiberious wants to merge 1 commit intopygae:masterfrom
utiberious:fix/issue-234-pdop-sympy

Conversation

@utiberious
Copy link
Copy Markdown
Contributor

Summary

Adds __hash__ and __ne__ to Pdop and Sdop so they can be used in sets and as dict keys, which is needed for proper SymPy integration (e.g. substitution, pattern matching).

Fixes #234

Changes

  • galgebra/dop.py: Add __hash__ and __ne__ to both Pdop and Sdop
    • Sdop.__hash__ uses hash(self.terms)
    • Pdop.__hash__ uses hash(tuple(sorted(self.pdiffs.items())))
  • test/test_differential_ops.py: Add tests for hashability, set membership, and dict key usage

Test plan

  • Unit tests pass
  • Full CI with nbval notebooks passes

Add __hash__ and __ne__ methods to Pdop and Sdop classes so they can
be used in sets, dictionaries, and other contexts that require hashable
objects. This is a step toward better integration with SymPy's
expression system.

Fixes #8
@utensil
Copy link
Copy Markdown
Member

utensil commented Mar 30, 2026

Thanks for working on this. Before we go further though, there's prior art worth building on: Eric Wieser started the right approach in #234 (branch pdop-expr-2). His idea was to make Pdop/Sdop proper SymPy objects by subclassing Expr directly via a DiffOpExpr(Expr, _BaseDop) base class, with DiffOpPartial, DiffOpMul(Expr, sympy.Mul), DiffOpAdd(Expr, sympy.Add), etc.

Why that matters: the simple __hash__/__ne__ approach in this PR has correctness issues that Eric's approach avoids. Specifically, Sdop.__hash__ uses hash(self.terms) but _merge_terms doesn't sort, so two algebraically equal Sdop instances built in different term order will compare equal but hash differently — a Python contract violation. Also Pdop({}) compares equal to S.One but their hashes will differ. When you subclass Expr, canonicalization and hashing come for free and these problems don't arise.

So the ask is: please pick up Eric's approach from #234 rather than patching around it. Eric should also be credited as a co-author on any commit that builds on his design.

Happy to help dig into the unfinished parts of #234 if useful.

@utiberious
Copy link
Copy Markdown
Contributor Author

Good point about the hash contract violation and Eric's prior work. The simple approach here does have the ordering bug you identified.

Closing this PR. The proper fix should build on Eric's pdop-expr-2 branch approach with Expr subclassing. That's a bigger undertaking and should credit Eric as co-author.

@utiberious utiberious closed this Mar 30, 2026
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.

2 participants