-
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
Move core_number to rustworkx-core #795
Conversation
Pull Request Test Coverage Report for Build 4079118655
💛 - Coveralls |
There was a cut and paste docs error in core_number which I also found in find_cycle and fixed both 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.
I left some minor comments. Most of them are easy to address. The harder things are:
- Should be remove the parallel sorting of edges? I am not as confident about that
- We probably should handle the invalid inputs now that we are changing the code
cores.insert(*k, k_deg); | ||
degree_map.insert(*k, k_deg); | ||
} | ||
node_vec.sort_by_key(|k| degree_map.get(k)); |
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.
Food for thought: should we remove the parallel sort? It might be worth benchmarking for large graphs
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.
No opinion on it. I'll try a quick benchmark and see if it shows a difference.
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.
Ran it with heavy hex with low core number and complete with high core number on my local and there was less that 5% variation between sort_by_key
and par_sort_by_key
. Maybe for some other types of graphs it could be different.
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’d import Rayon and use par_sort_by_key
. Rayon is in the dependencies already, might as well use it as the previous code for consistency
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.
Ok. I'll put par_sort_by_key
back.
Ok. I'll leave the |
This PR moves the function
core_number
fromsrc/connectivity/core_number.rs
torustworkx-core/connectivity/core_number.rs
.