Equality comparison mapper by kaushikcfd · Pull Request #166 · inducer/pytato

kaushikcfd · 2021-10-13T18:46:56Z

Closes #163

test/test_pytato.py

kaushikcfd · 2021-10-13T20:22:44Z

This is ready. The time taken by the pytest CI here and on main seems almost equal, so the overhead of creating a mapper for every comparison doesn't seem significant.

inducer

Thanks for working on this! A few questions below, but generally this looks good.

pytato/transform.py

inducer · 2021-10-18T02:48:16Z

pytato/transform.py

+        raise NotImplementedError
+
+    def map_placeholder(self, expr1: Placeholder, expr2: Any) -> bool:
+        return (isinstance(expr2, Placeholder)


isinstance isn't precise enough, since the object might compare equal to its subclass. I think the types need to match. (Yes, that was a bug in the original __eq__.)

Went with expr1.__class__ is expr2.__class__

pytato/transform.py

inducer

Thanks! A few more superficial bits, then this is good to go.

inducer · 2021-10-18T23:05:25Z

pytato/equality.py

+
+    def handle_unsupported_array(self, expr1: Array,
+                                 expr2: Any) -> bool:
+        raise NotImplementedError


I think it'd be good to have an intelligible messages here.

Added the type name. Thanks!

inducer · 2021-10-18T23:05:40Z

pytato/equality.py

+        raise NotImplementedError
+
+    def map_foreign(self, expr1: Any, expr2: Any) -> bool:
+        raise NotImplementedError


I think it'd be good to have an intelligible messages here.

Added the type name. Thanks!

inducer · 2021-10-18T23:08:41Z

pytato/equality.py

+    from pytato.loopy import LoopyCall
+
+__doc__ = """
+.. currentmodule:: pytato.equality


Suggested change

.. currentmodule:: pytato.equality

(not needed)

inducer · 2021-10-18T23:09:44Z

pytato/equality.py

+if TYPE_CHECKING:
+    from pytato.loopy import LoopyCall
+
+__doc__ = """


Should this be hooked into the external docs somewhere?

inducer · 2021-10-18T23:11:13Z

pytato/equality.py

+# }}}
+
+
+def is_structurally_equivalent(expr1: ArrayOrNames, expr2: Any) -> bool:


Not loving the name. equivalent suggests something more restrictive than equal.

Why not just call EqualityComparisonMapper()(expr1, expr2) in __eq__?

Ok, got rid of it.

inducer · 2021-10-18T23:11:43Z

pytato/equality.py

+
+# {{{ EqualityComparisonMapper
+
+class EqualityComparisonMapper:


Suggested change

class EqualityComparisonMapper:

class EqualityComparer:

?

inducer · 2021-10-18T23:11:56Z

pytato/equality.py

+    .. note::
+
+        Complexity: :math:`O(N)`, where :math:`N` is the number of nodes in
+        *expr1*.


This could get moved to the EqualityComparer docstring.

inducer · 2021-10-18T23:12:43Z

pytato/equality.py

+# {{{ EqualityComparisonMapper
+
+class EqualityComparisonMapper:
+    def __init__(self) -> None:


Add a docstring justifying why this object exists (to host caches for equality comparison, and that comparison might be exponential without it).

Co-authored-by: Andreas Kloeckner <inform@tiker.net>

inducer · 2021-10-19T00:02:57Z

Thanks!

kaushikcfd force-pushed the EqualityComparisonMapper branch 2 times, most recently from 343b4d0 to 859b387 Compare October 13, 2021 19:24

kaushikcfd commented Oct 13, 2021

View reviewed changes

test/test_pytato.py Outdated Show resolved Hide resolved

kaushikcfd force-pushed the EqualityComparisonMapper branch 2 times, most recently from 74e42b4 to bd1c612 Compare October 13, 2021 20:17

kaushikcfd marked this pull request as ready for review October 13, 2021 20:18

kaushikcfd requested a review from inducer October 13, 2021 20:42

inducer reviewed Oct 18, 2021

View reviewed changes

kaushikcfd force-pushed the EqualityComparisonMapper branch 2 times, most recently from 7847d63 to 4ee85e3 Compare October 18, 2021 20:38

kaushikcfd requested a review from inducer October 18, 2021 20:44

kaushikcfd force-pushed the EqualityComparisonMapper branch from 4ee85e3 to 2b47d9a Compare October 18, 2021 20:51

inducer reviewed Oct 18, 2021

View reviewed changes

kaushikcfd and others added 4 commits October 18, 2021 18:49

implements EqualityComparer

c75711f

Co-authored-by: Andreas Kloeckner <inform@tiker.net>

tests EqualityComparer on intestine graphs

2bcd50a

Array.__eq__ should call pt.equality.EqualityComparer

629228b

Adds EqualityComparisonMapper to docs

1d31563

kaushikcfd force-pushed the EqualityComparisonMapper branch from 2b47d9a to 1d31563 Compare October 18, 2021 23:51

kaushikcfd requested a review from inducer October 18, 2021 23:56

inducer enabled auto-merge (rebase) October 19, 2021 00:02

inducer merged commit 6f6f995 into inducer:main Oct 19, 2021

alexfikl mentioned this pull request Nov 15, 2021

Add EqualityMapper inducer/pymbolic#74

Draft

11 tasks

		# }}}


		def is_structurally_equivalent(expr1: ArrayOrNames, expr2: Any) -> bool:


		# {{{ EqualityComparisonMapper

		class EqualityComparisonMapper:

Conversation

kaushikcfd commented Oct 13, 2021

Uh oh!

Uh oh!

kaushikcfd commented Oct 13, 2021

Uh oh!

inducer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

inducer commented Oct 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants