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.

Sign up for GitHub

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

Jump to bottom

Equality comparison mapper #166

Merged
merged 4 commits into from
Oct 19, 2021
Merged

Equality comparison mapper #166

merged 4 commits into from
Oct 19, 2021

Conversation

kaushikcfd
Copy link
Collaborator

Closes #163

@kaushikcfd kaushikcfd force-pushed the EqualityComparisonMapper branch 2 times, most recently from 343b4d0 to 859b387 Compare October 13, 2021 19:24
kaushikcfd
kaushikcfd commented Oct 13, 2021
View reviewed changes
test/test_pytato.py Outdated Show resolved Hide resolved
@kaushikcfd kaushikcfd force-pushed the EqualityComparisonMapper branch 2 times, most recently from 74e42b4 to bd1c612 Compare October 13, 2021 20:17
@kaushikcfd kaushikcfd marked this pull request as ready for review October 13, 2021 20:18
@kaushikcfd
Copy link
Collaborator Author

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
inducer reviewed Oct 18, 2021
View reviewed changes
Copy link
Owner

@inducer inducer left a comment

Choose a reason for hiding this comment

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

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

pytato/transform.py Outdated Show resolved Hide resolved
pytato/transform.py Outdated Show resolved Hide resolved
pytato/transform.py Outdated
raise NotImplementedError

def map_placeholder(self, expr1: Placeholder, expr2: Any) -> bool:
return (isinstance(expr2, Placeholder)
Copy link
Owner

Choose a reason for hiding this comment

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

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__.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Went with expr1.__class__ is expr2.__class__

pytato/transform.py Outdated Show resolved Hide resolved
pytato/transform.py Outdated Show resolved Hide resolved
@kaushikcfd kaushikcfd force-pushed the EqualityComparisonMapper branch 2 times, most recently from 7847d63 to 4ee85e3 Compare October 18, 2021 20:38
inducer
inducer reviewed Oct 18, 2021
View reviewed changes
Copy link
Owner

@inducer inducer left a comment

Choose a reason for hiding this comment

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

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

pytato/equality.py Outdated

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added the type name. Thanks!

pytato/equality.py Outdated
raise NotImplementedError

def map_foreign(self, expr1: Any, expr2: Any) -> bool:
raise NotImplementedError
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added the type name. Thanks!

pytato/equality.py Outdated
from pytato.loopy import LoopyCall

__doc__ = """
.. currentmodule:: pytato.equality
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
.. currentmodule:: pytato.equality

(not needed)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

pytato/equality.py
if TYPE_CHECKING:
from pytato.loopy import LoopyCall

__doc__ = """
Copy link
Owner

Choose a reason for hiding this comment

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

Should this be hooked into the external docs somewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

pytato/equality.py Outdated
# }}}


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

Choose a reason for hiding this comment

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

  • Not loving the name. equivalent suggests something more restrictive than equal.
  • Why not just call EqualityComparisonMapper()(expr1, expr2) in __eq__?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, got rid of it.

pytato/equality.py Outdated

# {{{ EqualityComparisonMapper

class EqualityComparisonMapper:
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
class EqualityComparisonMapper:
class EqualityComparer:

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure.

pytato/equality.py Outdated
Comment on lines 230 to 233
.. note::

Complexity: :math:`O(N)`, where :math:`N` is the number of nodes in
*expr1*.
Copy link
Owner

Choose a reason for hiding this comment

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

This could get moved to the EqualityComparer docstring.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

pytato/equality.py
# {{{ EqualityComparisonMapper

class EqualityComparisonMapper:
def __init__(self) -> None:
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@inducer inducer enabled auto-merge (rebase) October 19, 2021 00:02
@inducer
Copy link
Owner

inducer commented Oct 19, 2021

Thanks!

@inducer inducer merged commit 6f6f995 into inducer:main Oct 19, 2021
@alexfikl alexfikl mentioned this pull request Nov 15, 2021
Draft
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@inducer inducer Awaiting requested review from inducer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Equality comparison of DAGs can have exponential complexity
2 participants
@kaushikcfd @inducer