Rename misleading topological_sort to bfs_nodes #1590
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BenWeber42
force-pushed
the
fix-erronous-topological-sort
branch
5 times, most recently
from
6b2f543
to
355cabd
Compare
BenWeber42
force-pushed
the
fix-erronous-topological-sort
branch
from
355cabd
to
fe23f49
Compare
tbennun
reviewed
Jun 20, 2024
| @@ -1848,7 +1848,7 @@ def get_next_scope_entries(self, sdfg, dfg, scope_entry): | |||
| parent_scope = dfg.scope_subgraph(parent_scope_entry) | |||
|
|
|||
| # Get all scopes from the same level | |||
| all_scopes = [node for node in parent_scope.topological_sort() if isinstance(node, dace.sdfg.nodes.EntryNode)] | |||
| all_scopes = [node for node in parent_scope.bfs_nodes() if isinstance(node, dace.sdfg.nodes.EntryNode)] | |||
There was a problem hiding this comment.
why is this not .nodes()?
There was a problem hiding this comment.
hmm, a bit of a guess, but maybe because of the next line:
return all_scopes[all_scopes.index(scope_entry) + 1:].nodes() might return a different order.
tbennun
reviewed
Jun 20, 2024
| @@ -2096,7 +2096,7 @@ def get_next_scope_entries(self, dfg, scope_entry): | |||
|
|
|||
| # Get all non-sequential scopes from the same level | |||
| all_scopes = [ | |||
There was a problem hiding this comment.
I am not exactly sure why this does not use scope_subgraph instead.
There was a problem hiding this comment.
hmm, I'll try to look into this later. But for now I'd like to wrap up this PR. If we should change this, I think it should be in a separate PR.
But thanks for pointing it out. Looks like there's a potential to improve the code there.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is currently work in progress to see how we can best fix this misleading naming.
Fixes #1560
Since #1560 is still in flux, we have to make sure the PR stays in sync with what we are discussing in #1560.
Additionally, at the call-sites of the previous topological_sort (before renaming), there are various comments to use a topoligical sort. After the renaming, they become misleading, so we should probably fix/improve those comments.