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 node type counter and tests #507

Closed
wants to merge 10 commits into from
Closed

Add node type counter and tests #507

wants to merge 10 commits into from

Conversation

kajalpatelinfo
Copy link
Contributor

Replacing #489 .

This PR builds off the work in #489 by adding tests to check the count of nodes and types of nodes in a DAG. NodeCountMapper counts duplicate nodes once.

@kajalpatelinfo kajalpatelinfo marked this pull request as ready for review June 18, 2024 01:47
majosm
majosm reviewed Jun 18, 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.

Looks great! Just added a couple of minor thoughts.

pytato/analysis/__init__.py Show resolved Hide resolved
test/test_pytato.py Outdated Show resolved Hide resolved
@kajalpatelinfo kajalpatelinfo mentioned this pull request Jun 23, 2024
Merged
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.

A couple more suggestions, otherwise looks good to go on my end.

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
@majosm
Copy link
Collaborator

majosm commented Jun 28, 2024

Looks good to me! @inducer?

@majosm
Copy link
Collaborator

majosm commented Jul 15, 2024

Hey @kajalpatelinfo , it looks like there's a test in test_codegen.py that depends on the previous behavior of get_num_nodes. I talked to @inducer and we were thinking maybe we should just go straight to #508 instead of merging this one separately. Then we can just add count_duplicates=True to the test instead of figuring out what to do with it here in the meantime. Sound OK?

@kajalpatelinfo
Copy link
Contributor Author

Yep, that makes sense!! I'll go over the changes to #508 tonight. Thank you!

@inducer
Copy link
Owner

inducer commented Jul 18, 2024

Closing, superseded by #508.

@inducer inducer closed this Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@majosm majosm majosm left review comments

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 @majosm @inducer