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

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

Closes#163

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.

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

+ 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__

inducer

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

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

+
+ 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

+ 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

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


Suggested change
.. currentmodule:: pytato.equality
(not needed)

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

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


Should this be hooked into the external docs somewhere?

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

+# }}}
+
+
+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

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


Suggested change
classEqualityComparisonMapper:
classEqualityComparer:
?

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

+ .. 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

+# {{{ 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 859b387Compare October 13, 2021 19:24

kaushikcfd commented Oct 13, 2021
View reviewed changes

Comment threadtest/test_pytato.py Outdated

kaushikcfd force-pushed the EqualityComparisonMapper branch 2 times, most recently from 74e42b4 to bd1c612Compare 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 4ee85e3Compare 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 2b47d9aCompare October 18, 2021 20:51

inducer reviewed Oct 18, 2021
View reviewed changes

kaushikcfdand 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 1d31563Compare 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:mainOct 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