-
Notifications
You must be signed in to change notification settings - Fork 145
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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))/2
as 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 |
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.
I think this is good to go
This adds a generator for the hyperbolic random graph model. There are two issues I wasn't able to solve:
Data
trait 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.