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

add PytatoKeyBuilder, persistent_dict test #459

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

matthiasdiener
Copy link
Collaborator

@matthiasdiener matthiasdiener commented Sep 25, 2023

Alternative to #457.

See #547 for a more in-depth discussion.

Please squash

@matthiasdiener matthiasdiener self-assigned this Sep 25, 2023
@matthiasdiener matthiasdiener force-pushed the PytatoKeyBuilder branch 2 times, most recently from b2d71a3 to caa5a75 Compare September 25, 2023 22:41
Comment on lines 468 to 501
def update_for_pymbolic_expression(self, key_hash: Any, key: Any) -> None:
if key is None:
self.update_for_NoneType(key_hash, key) # type: ignore[no-untyped-call]
else:
PersistentHashWalkMapper(key_hash)(key)

update_for_Product = update_for_pymbolic_expression # noqa: N815
update_for_Sum = update_for_pymbolic_expression # noqa: N815
update_for_If = update_for_pymbolic_expression # noqa: N815
update_for_LogicalOr = update_for_pymbolic_expression # noqa: N815
update_for_Call = update_for_pymbolic_expression # noqa: N815
update_for_Comparison = update_for_pymbolic_expression # noqa: N815
update_for_Quotient = update_for_pymbolic_expression # noqa: N815
update_for_Power = update_for_pymbolic_expression # noqa: N815
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This could probably be improved, but I'm not sure how.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps inducer/pymbolic#125 resolves 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.

I'm guessing this PR is waiting for inducer/pymbolic#125 ?

Copy link
Owner

Choose a reason for hiding this comment

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

Yep, inducer/pymbolic#125 has resolved this, this bit should no longer be necessary. 😁

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed in 70a6887

pytato/analysis/__init__.py Outdated Show resolved Hide resolved
@matthiasdiener matthiasdiener changed the title add PytatoKeyBuilder add PytatoKeyBuilder, persistent_dict test Sep 25, 2023
@matthiasdiener matthiasdiener force-pushed the PytatoKeyBuilder branch 2 times, most recently from 72e5569 to b31657f Compare November 7, 2023 16:48
@matthiasdiener matthiasdiener marked this pull request as ready for review January 26, 2024 15:23
@matthiasdiener
Copy link
Collaborator Author

This is ready for review @inducer. The current version is able to hash the DAG of a 2-rank smoke_test_ks_3d run.

@matthiasdiener
Copy link
Collaborator Author

A gentle ping for review @inducer. This PR is helpful with e.g. #498.

@inducer
Copy link
Owner

inducer commented Sep 21, 2024

See also #547.

Comment on lines 583 to 584
# CL Array
self.rec(key_hash, key.get())
Copy link
Owner

Choose a reason for hiding this comment

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

Could you use an isinstance check to make sure that this only hashes the intended types?

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 in 70a6887

Comment on lines 468 to 501
def update_for_pymbolic_expression(self, key_hash: Any, key: Any) -> None:
if key is None:
self.update_for_NoneType(key_hash, key) # type: ignore[no-untyped-call]
else:
PersistentHashWalkMapper(key_hash)(key)

update_for_Product = update_for_pymbolic_expression # noqa: N815
update_for_Sum = update_for_pymbolic_expression # noqa: N815
update_for_If = update_for_pymbolic_expression # noqa: N815
update_for_LogicalOr = update_for_pymbolic_expression # noqa: N815
update_for_Call = update_for_pymbolic_expression # noqa: N815
update_for_Comparison = update_for_pymbolic_expression # noqa: N815
update_for_Quotient = update_for_pymbolic_expression # noqa: N815
update_for_Power = update_for_pymbolic_expression # noqa: N815
Copy link
Owner

Choose a reason for hiding this comment

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

Yep, inducer/pymbolic#125 has resolved this, this bit should no longer be necessary. 😁

Comment on lines 586 to 607
update_for_BitwiseAnd = LoopyKeyBuilder.update_for_pymbolic_expression # noqa: N815
update_for_BitwiseNot = LoopyKeyBuilder.update_for_pymbolic_expression # noqa: N815
update_for_BitwiseOr = LoopyKeyBuilder.update_for_pymbolic_expression # noqa: N815
update_for_BitwiseXor = LoopyKeyBuilder.update_for_pymbolic_expression # noqa: N815
update_for_Call = LoopyKeyBuilder.update_for_pymbolic_expression # noqa: N815
update_for_CallWithKwargs = LoopyKeyBuilder.update_for_pymbolic_expression # noqa: N815
update_for_Comparison = LoopyKeyBuilder.update_for_pymbolic_expression # noqa: N815
update_for_If = LoopyKeyBuilder.update_for_pymbolic_expression # noqa: N815
update_for_FloorDiv = LoopyKeyBuilder.update_for_pymbolic_expression # noqa: N815
update_for_LeftShift = LoopyKeyBuilder.update_for_pymbolic_expression # noqa: N815
update_for_LogicalAnd = LoopyKeyBuilder.update_for_pymbolic_expression # noqa: N815
update_for_LogicalNot = LoopyKeyBuilder.update_for_pymbolic_expression # noqa: N815
update_for_LogicalOr = LoopyKeyBuilder.update_for_pymbolic_expression # noqa: N815
update_for_Lookup = LoopyKeyBuilder.update_for_pymbolic_expression # noqa: N815
update_for_Power = LoopyKeyBuilder.update_for_pymbolic_expression # noqa: N815
update_for_Product = LoopyKeyBuilder.update_for_pymbolic_expression # noqa: N815
update_for_Quotient = LoopyKeyBuilder.update_for_pymbolic_expression # noqa: N815
update_for_Remainder = LoopyKeyBuilder.update_for_pymbolic_expression # noqa: N815
update_for_RightShift = LoopyKeyBuilder.update_for_pymbolic_expression # noqa: N815
update_for_Subscript = LoopyKeyBuilder.update_for_pymbolic_expression # noqa: N815
update_for_Sum = LoopyKeyBuilder.update_for_pymbolic_expression # noqa: N815
update_for_Variable = LoopyKeyBuilder.update_for_pymbolic_expression # noqa: N815
Copy link
Owner

Choose a reason for hiding this comment

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

All this can go now that inducer/pymbolic#125 is in.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed in 70a6887

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