-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
refactor: rasa/shared/core/training_data/visualization.py #12544
Conversation
In the refactored version, In function(_remove_auxiliary_nodes), I've replaced the conversion of graph.predecessors(i) into a list with the direct usage of the generator. Additionally, I've introduced the predecessors_seen set to efficiently keep track of seen predecessors. When a duplicated predecessor is found, we can remove the node and break out of the inner loop. This optimization reduces the time complexity of checking for duplicated nodes to approximately O((TMP_NODE_ID - special_node_idx) + out_degree(node)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 Thanks for the contribution can you also add a unit test for _remove_auxiliary_nodes
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the unit test 👍
In this commit, I have implemented example test cases to test method: _remove_auxiliary_nodes in @pytest.mark.parametrize. Also,fixed the import error of networkx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 Thanks for the contribution just fix the the ruff errors
Hey @Urkem, Done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one small comment :)
In this version, I have removed break statement from the loop so it can remove all the predecessors nodes which are in predecessors_seen without breaking the loop.
Hey @Urkem, |
Hey @Urkem, |
Hey @arjun-234 I think these changes are making a regression test fail at, |
Hey @vcidst, |
@arjun-234 I see, it might be a flaky test in that case. I'll re-run the CI and see if the problem persists |
@vcidst Thanks :) |
In the refactored version, In function(_remove_auxiliary_nodes), I've replaced the conversion of graph.predecessors(i) into a list with the direct usage of the generator. Additionally, I've introduced the predecessors_seen set to efficiently keep track of seen predecessors. When a duplicated predecessor is found, we can remove the node and break out of the inner loop.
Proposed changes:
Status (please check what you already did):