Implementation of the StableDag data structure #21
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
…s based on the StableGraph data structure in petgraph. StableDag contains almost the same functionality as Dag. However, the following methods are not implemented because StableGraph in petgraph does not provide the necessary methods: raw_nodes() -> Because StableGraph lacks raw_nodes(). node_weights_mut() -> Because StableGraph lacks node_weights_mut(). raw_edges() -> Because StableGraph lacks raw_edges(). edge_weights_mut() -> Because StableGraph lacks edge_weights_mut().
mitchmindtree
requested changes
Oct 4, 2018
There was a problem hiding this comment.
Thanks so much @davenza, this is really great stuff! I've been meaning to get around to this for a while, I can definitely see myself using this down the track.
I just have a couple very minor things to clean up - otherwise I'm more than happy to merge!
So sorry for the delay by the way, it's getting a little overwhelming maintaining what must be ~50 crates by now, whilst also working at my own business! Thanks for being patient with me :)
- Removed commented methods not implemented by petgraph. - Re-formated the stabledag.rs file to have the same format as lib.rs: - In the where clauses, it uses the same space formatting as in lib.rs. - Re-formated some comments to fit in 100 columns. - Corrected typo in the description of the module.
… way to the "stable_graph" feature in petgraph.
|
I forgot to include conditional compilation in the tests, but it should be solved in cce14bc. |
|
Any updates on this PR? I would love to help if there are any specific issues, especially in regard to RustAudio/dsp-chain#103. |
|
Thanks for the reminder @fwcd! |
mitchmindtree
approved these changes
Sep 14, 2019
mtreinish
added a commit
to Qiskit/rustworkx
that referenced
this pull request
Jan 26, 2020
This commit migrates the internal graph representation of a PyDAG object from daggy to use stabledigraph in petgraph directly. This removes an abstraction layer from the stack and also makes it easier to use the dag in python since the indexes are stable now. The implementation of the petgraph trait implementations is borrowed heavily from mitchmindtree/daggy#21 which added a stable graph dag implementation to daggy. If/when daggy pushes a new release that includes this implementation and keeps up to date with the latest version of petgraph it will probably be worth switching back to that newer version of daggy. The current issue is most of the petgraph algorithms do not work with StableGraph types and instead rely solely on Graph types. As a workaround for this we will need to add local implementations of these algorithms in the repo. Then after we fix these issues upstream and include them in a release we can switch back to petgraph's implementation. Also, right now the scaling performance of this is limited by the cycle detection. At sizes above 10,000 nodes it will take more than a second to add a child, parent, or edge. To avoid this we can either remove the runtime cycle detection on every add operation. Or alternatively looking into adapting the rolling cycle detection we were previously getting for free from daggy. Fixes #6
1 task
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 an early implementation of a StableDag.The code is almost copied from the Dag structure, but using StableGraph instead of Graph. As indicated in the original commit, there are 4 methods that are not implemented because StableGraph does not provide the necessary functions:
I included the same set of tests for StableDag that is used in the Dag structure in the files
tests/add_edges_stable.rs
tests/iterators_stable.rs
tests/walkers_stable.rs
I also included the tests in tests/stable_indices to test that node/edge indices do not change when we remove an edge/node.
I am pretty new to Rust, so it may need some better code / project organization.