Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not simplify in overloaded operators, type mappers #152

Merged
merged 25 commits into from
Nov 6, 2024

Conversation

inducer
Copy link
Owner

@inducer inducer commented Oct 6, 2024

Closes #11.
Closes #149.

What finally tipped the balance is that the overload-y operators are hard to precisely type, which made typing various downstream user packages unnecessarily hard.

cc @kaushikcfd

@inducer inducer mentioned this pull request Oct 6, 2024
1 task
@inducer inducer changed the title Do not simplify in overloaded operators Do not simplify in overloaded operators, type mappers Oct 8, 2024
@inducer inducer force-pushed the no-simp-in-operators branch 3 times, most recently from 0c76ffa to a787ba3 Compare October 21, 2024 02:54
@inducer inducer force-pushed the no-simp-in-operators branch 2 times, most recently from 2c129f2 to f41f938 Compare October 21, 2024 03:04
Copy link
Collaborator

@alexfikl alexfikl left a comment

Choose a reason for hiding this comment

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

Took a swift look over this and it looks good to me. Downstreams seem very upset with it though :(

pymbolic/geometric_algebra/__init__.py Outdated Show resolved Hide resolved
pymbolic/geometric_algebra/__init__.py Show resolved Hide resolved
pymbolic/mapper/constant_folder.py Outdated Show resolved Hide resolved
pymbolic/mapper/flattener.py Show resolved Hide resolved
@inducer
Copy link
Owner Author

inducer commented Oct 21, 2024

Downstreams seem very upset with it though :(

They sure are. I'm trying to figure out whether the damage can be controlled or whether we need to do this in a more piecemeal fashion.

@inducer inducer force-pushed the no-simp-in-operators branch 2 times, most recently from f77d984 to 0d03277 Compare October 21, 2024 17:31
@alexfikl
Copy link
Collaborator

alexfikl commented Oct 23, 2024

pytential seems to work with this patch (didn't run all the testsuite though)

diff --git a/pymbolic/geometric_algebra/__init__.py b/pymbolic/geometric_algebra/__init__.py
index fbc7e30..4291b74 100644
--- a/pymbolic/geometric_algebra/__init__.py
+++ b/pymbolic/geometric_algebra/__init__.py
@@ -526,6 +526,7 @@ class MultiVector(Generic[CoeffT]):
"""

space: Space
+    mapper_method = "map_multivector"

# {{{ construction

diff --git a/pymbolic/mapper/coefficient.py b/pymbolic/mapper/coefficient.py
index 99516f0..b09b4f6 100644
--- a/pymbolic/mapper/coefficient.py
+++ b/pymbolic/mapper/coefficient.py
@@ -100,6 +100,9 @@ class CoefficientCollector(Mapper):
return {1: expr}

def map_constant(self, expr):
+        if expr == 0:
+            return {}
+
return {1: expr}

def map_algebraic_leaf(self, expr):

EDIT: Well, that was wrong, there are other errors too :(
EDIT: It seems to be missing some handling for MultiVector. Not sure why that worked before..
EDIT: Ugh, all the map_multivector need to be changed to map_multi_vector. Better to overwrite the default mapper_method for it?

@inducer inducer force-pushed the no-simp-in-operators branch 2 times, most recently from d776b9b to 543e1b1 Compare November 3, 2024 21:17
@inducer inducer force-pushed the no-simp-in-operators branch 3 times, most recently from 6964471 to b4b87cb Compare November 6, 2024 21:36
@inducer inducer marked this pull request as ready for review November 6, 2024 21:54
@inducer inducer enabled auto-merge (rebase) November 6, 2024 22:09
@inducer inducer merged commit fd9d07d into main Nov 6, 2024
11 checks passed
@inducer inducer deleted the no-simp-in-operators branch November 6, 2024 23:01
@inducer inducer mentioned this pull request Nov 7, 2024
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.

Precisely type the mappers Sum and Product evaluates zeros and ones
3 participants