Skip to content
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

Merged
merged 13 commits into from
Feb 2, 2023
Merged

Conversation

enavarro51
Copy link
Contributor

This PR moves the function core_number from src/connectivity/core_number.rs to rustworkx-core/connectivity/core_number.rs.

@coveralls
Copy link

coveralls commented Jan 28, 2023

Pull Request Test Coverage Report for Build 4079118655

  • 68 of 68 (100.0%) changed or added relevant lines in 2 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.03%) to 97.081%

Files with Coverage Reduction New Missed Lines %
src/shortest_path/all_pairs_bellman_ford.rs 2 98.88%
Totals Coverage Status
Change from base Build 4075927635: 0.03%
Covered Lines: 13736
Relevant Lines: 14149

💛 - Coveralls

@enavarro51
Copy link
Contributor Author

There was a cut and paste docs error in core_number which I also found in find_cycle and fixed both here.

Copy link
Collaborator

@IvanIsCoding IvanIsCoding left a 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

rustworkx-core/src/connectivity/find_cycle.rs Show resolved Hide resolved
tests/rustworkx_tests/graph/test_core_number.py Outdated Show resolved Hide resolved
tests/rustworkx_tests/graph/test_core_number.py Outdated Show resolved Hide resolved
tests/rustworkx_tests/graph/test_core_number.py Outdated Show resolved Hide resolved
tests/rustworkx_tests/graph/test_core_number.py Outdated Show resolved Hide resolved
cores.insert(*k, k_deg);
degree_map.insert(*k, k_deg);
}
node_vec.sort_by_key(|k| degree_map.get(k));
Copy link
Collaborator

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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

Copy link
Contributor Author

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.

@enavarro51
Copy link
Contributor Author

Ok. I'll leave the implicitly assumes for later and will open an issue for it.

@IvanIsCoding IvanIsCoding added the automerge Queue a approved PR for merging label Feb 2, 2023
@mergify mergify bot merged commit 1ffa4d4 into Qiskit:main Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Queue a approved PR for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants