Add hyperbolic random graph model generator #1196
Conversation
…kit#1195) In the recently merged Qiskit#1192 a new generic DAG longest_path function was added to rustworkx-core. However, the trait bounds on the function were a bit tighter than they needed to be. The traits were forcing NodeId to be of a NodeIndex type and this wasn't really required. The only requirement that the NodeId type can be put on a hashmap and do a partial compare (that implements Hash, Eq, and PartialOrd). Also the IntoNeighborsDirected wasn't required because it's methods weren't ever used. This commit loosens the traits bounds to facilitate this. At the same time this also simplifies the code structure a bit to reduce the separation of the rust code structure in the rustworkx crate using longest_path().
Pull Request Test Coverage Report for Build 9189895728Details
💛 - Coveralls |
There was a problem hiding this comment.
Overall this is looking pretty good! I wouldn't worry to much about the petgraph's traits, this seems to make sense to me.
I don't have much to comment on Hyperbolic Random Graphs as it is not our core area of expertise, but I did add some comments about floating-point arithmetic:
- Intsead of multiplying
cosh(a)*cosh(b), I'd look for(cosh(a + b) - cos(a - b))/2as this can might be able to boost the accuracy. Same applies forsinh. Check with https://herbie.uwplse.org/ if you want to be sure - I'd also be careful with comparisons involving floats
Lastly, our pure Rust documentation doesn't support LaTex as we'd need to add KaTeX with something like https://docs.rs/rustdoc-katex-demo/0.1.5/rustdoc_katex_demo/. So you can leave it as is and we'll figure it out later
releasenotes/notes/hyperbolic-random-graph-d85c115930d8ac08.yaml
Outdated
Show resolved
Hide resolved
|
I believe I have fixed everything that was talked about. I don't see how to rewrite this version of the hyperbolic distance in a numerical friendly way. That said, I generalized the code to allow for any valid dimension of the hyperbolic space. To do so, it was easier to change the coordinate system and as a bonus, the hyperbolic distance has a more numerical friendly expression. |
|
I have no idea why Coverall now fails. Do you know why that is? |
I think an update broke our CI, I'll try to fix it |
This adds a generator for the hyperbolic random graph model. There are two issues I wasn't able to solve:
Datatrait and return an error (runtime) when the graph is directed. It's clear to me that it should be a compile error, but I'm not familiar enough with traits andpetgraph.Related to #150. This is my first contribution and I'm very open to suggestions.