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

Consistency of show_dot_graph and get_num_nodes #504

Open
majosm opened this issue Jun 17, 2024 · 4 comments
Open

Consistency of show_dot_graph and get_num_nodes #504

majosm opened this issue Jun 17, 2024 · 4 comments

Comments

@majosm
Copy link
Collaborator

majosm commented Jun 17, 2024

A question came up while revisiting #489 with @kajalpatelinfo: should show_dot_graph be consistent with get_num_nodes, in the sense that the number of nodes displayed in the graph matches what get_num_nodes computes? It looks like show_dot_graph's code is based on CopyMapper, which ignores duplicates, but currently get_num_nodes uses id(expr) for a hash key, meaning it counts duplicates separately.

I'm tempted to think they should behave the same, at least by default. It might actually be useful to give them both an option to include/exclude duplicates? Thoughts @inducer?

@inducer
Copy link
Owner

inducer commented Jun 17, 2024

I think get_num_nodes should be adapted to only count non-duplicates. I'm skeptical that including duplicates (at least the naive way) is useful functionality, as it could very easily have exponential complexity. As for non-naive, maybe that's useful? But it should probably not be the default.

@majosm
Copy link
Collaborator Author

majosm commented Jun 17, 2024

I think get_num_nodes should be adapted to only count non-duplicates. I'm skeptical that including duplicates (at least the naive way) is useful functionality, as it could very easily have exponential complexity. As for non-naive, maybe that's useful? But it should probably not be the default.

What's naive/non-naive here? No cache vs. cache-by-id? Or something else?

@inducer
Copy link
Owner

inducer commented Jun 17, 2024

Naive = no cache.

@kajalpatelinfo
Copy link
Contributor

Okay, thank you both!

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

No branches or pull requests

3 participants