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

Add duplicate node counter functionality and tests #508

Merged
merged 34 commits into from
Aug 1, 2024
Merged

Add duplicate node counter functionality and tests #508

merged 34 commits into from
Aug 1, 2024

Conversation

kajalpatelinfo
Copy link
Contributor

Building off #507.

This PR builds off #507 by adding functionality and tests to count duplicates however many times they appear in the DAG. The default functionality is to not count duplicates.

@kajalpatelinfo kajalpatelinfo marked this pull request as ready for review June 25, 2024 02:49
@kajalpatelinfo
Copy link
Contributor Author

@majosm Please review when convenient!

majosm
majosm reviewed Jun 26, 2024
View reviewed changes
Copy link
Collaborator

@majosm majosm 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 quick glance through this; looks good so far! I'll give it a closer look once #507 gets merged.

pytato/analysis/__init__.py Outdated Show resolved Hide resolved
@majosm majosm mentioned this pull request Jul 15, 2024
Closed
@inducer
Copy link
Owner

inducer commented Jul 18, 2024

@majosm and I discussed this, and we agreed that we'd prefer to merge this and #507 in one go, so I'll go ahead and close #507.

Could you look at the test and linter failures?

Unsubscribing... @-mention or request review once it's ready for a look or needs attention.

@kajalpatelinfo
Copy link
Contributor Author

Sounds good, I'll fix those errors today. Thank you!

majosm
majosm reviewed Jul 23, 2024
View reviewed changes
Copy link
Collaborator

@majosm majosm left a comment

Choose a reason for hiding this comment

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

Mostly looks good, just some suggestions for doc stuff and a couple of the tests.

FYI there will likely be some type annotations to fix up now that #527 has gone in.

pytato/analysis/__init__.py Outdated Show resolved Hide resolved
pytato/analysis/__init__.py Outdated Show resolved Hide resolved
pytato/analysis/__init__.py Outdated Show resolved Hide resolved
pytato/analysis/__init__.py Outdated Show resolved Hide resolved
pytato/analysis/__init__.py Outdated Show resolved Hide resolved
test/test_pytato.py Outdated Show resolved Hide resolved
test/test_pytato.py Outdated Show resolved Hide resolved
test/test_pytato.py Outdated Show resolved Hide resolved
test/test_pytato.py Outdated Show resolved Hide resolved
test/test_pytato.py Outdated Show resolved Hide resolved
majosm
majosm approved these changes Jul 26, 2024
View reviewed changes
Copy link
Collaborator

@majosm majosm left a comment

Choose a reason for hiding this comment

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

👍

test/testlib.py Outdated Show resolved Hide resolved
@majosm majosm requested a review from inducer July 26, 2024 19:45
@majosm
Copy link
Collaborator

majosm commented Jul 26, 2024

How's this look @inducer?

inducer
inducer reviewed Jul 29, 2024
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! Almost looks good, not far now. 👍

pytato/analysis/__init__.py
return ncm.expr_type_counts


def get_num_nodes(
Copy link
Owner

Choose a reason for hiding this comment

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

This breaks compatibility (= True would retain compatibility).

Maybe deprecate not specifying the argument. (@majosm can explain)

pytato/analysis/__init__.py Show resolved Hide resolved
pytato/analysis/__init__.py Outdated Show resolved Hide resolved
pytato/analysis/__init__.py Outdated Show resolved Hide resolved
majosm
majosm reviewed Jul 29, 2024
View reviewed changes
pytato/analysis/__init__.py Outdated Show resolved Hide resolved
inducer
inducer reviewed Jul 31, 2024
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! One and a half more wrinkles, and this should be ready to go.

pytato/analysis/__init__.py Show resolved Hide resolved
test/testlib.py Show resolved Hide resolved
test/testlib.py Outdated Show resolved Hide resolved
@inducer inducer merged commit 86733af into inducer:main Aug 1, 2024
11 checks passed
@inducer
Copy link
Owner

inducer commented Aug 1, 2024

Thanks!

@kajalpatelinfo kajalpatelinfo deleted the duplicate_node_counts branch August 14, 2024 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@inducer inducer inducer left review comments

@majosm majosm majosm approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@kajalpatelinfo @inducer @majosm