-
Notifications
You must be signed in to change notification settings - Fork 17
use more than 1byte for hashing based edge weights #81
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
Conversation
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.
Pull Request Overview
This PR refactors the edge weight generation system to support hashing-based edge weight generation and improve the test infrastructure. The main focus is implementing a deterministic, hash-based edge weight generator that ensures consistent weights for undirected edges regardless of edge direction.
- Introduced
PerEdgeWeightGeneratorbase class withEdgeRangesupport for cleaner edge iteration - Implemented hashing-based edge weight generation with proper undirected edge support
- Refactored test fixtures from specific "UniformWeight" to general "GeneralEdgeWeight" to support multiple weight generation types
- Added comprehensive test coverage for hashing-based weight generation
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/edge_weights/edge_generation_test.cpp | Renamed test fixture to GeneralEdgeWeightTestFixture and added four new test cases for hashing-based edge weight generation across both edgelist and CSR representations |
| kagen/generators/generator.cpp | Updated CreateEdgeWeightGenerator to pass vertex_range parameter to HashingBasedEdgeWeightGenerator constructor |
| kagen/edgeweight_generators/per_edge_weight_generator.h | Refactored to use EdgeRange for unified edge iteration, added vertex_range_ member and constructor parameter |
| kagen/edgeweight_generators/hashing_based_generator.h | Updated constructor signature to accept VertexRange parameter |
| kagen/edgeweight_generators/hashing_based_generator.cpp | Implemented improved hash-based weight generation with undirected edge support (swapping u,v), better hash distribution using 128-bit arithmetic, and fallback behavior when xxHash is unavailable |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| std::string name = std::get<0>(GetParam()); | ||
| GeneratorFunc generate = std::get<1>(GetParam()); | ||
| const SInt n = 1000; | ||
| const SInt m = 16 * n; | ||
| const WeightRange weight_range{1, 100}; |
Copilot
AI
Nov 7, 2025
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.
The variables name and weight_range are extracted from test parameters but never used in the test body. Consider removing them to avoid confusion.
| std::string name = std::get<0>(GetParam()); | ||
| GeneratorFunc generate = std::get<1>(GetParam()); | ||
| const SInt n = 1000; | ||
| const SInt m = 16 * n; | ||
| const WeightRange weight_range{1, 100}; | ||
|
|
||
| kagen::KaGen generator(MPI_COMM_WORLD); | ||
| generator.UseEdgeListRepresentation(); | ||
| generator.ConfigureEdgeWeightGeneration( | ||
| kagen::EdgeWeightGeneratorType::HASHING_BASED, weight_range.first, weight_range.second); | ||
|
|
||
| Graph graph = generate(generator, n, m); | ||
| check_backedge_weight(graph); | ||
| } | ||
|
|
||
| TEST_P(GeneralEdgeWeightTestFixture, hashing_based_correct_backedge_weights_csr_representation) { | ||
| std::string name = std::get<0>(GetParam()); | ||
| GeneratorFunc generate = std::get<1>(GetParam()); | ||
| const SInt n = 1000; | ||
| const SInt m = 16 * n; | ||
| const WeightRange weight_range{1, 100}; | ||
|
|
||
| kagen::KaGen generator(MPI_COMM_WORLD); | ||
| generator.UseCSRRepresentation(); | ||
| generator.ConfigureEdgeWeightGeneration( | ||
| kagen::EdgeWeightGeneratorType::HASHING_BASED, weight_range.first, weight_range.second); |
Copilot
AI
Nov 7, 2025
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.
The variables name and weight_range are extracted from test parameters but never used in the test body. Consider removing them to avoid confusion.
| std::string name = std::get<0>(GetParam()); | |
| GeneratorFunc generate = std::get<1>(GetParam()); | |
| const SInt n = 1000; | |
| const SInt m = 16 * n; | |
| const WeightRange weight_range{1, 100}; | |
| kagen::KaGen generator(MPI_COMM_WORLD); | |
| generator.UseEdgeListRepresentation(); | |
| generator.ConfigureEdgeWeightGeneration( | |
| kagen::EdgeWeightGeneratorType::HASHING_BASED, weight_range.first, weight_range.second); | |
| Graph graph = generate(generator, n, m); | |
| check_backedge_weight(graph); | |
| } | |
| TEST_P(GeneralEdgeWeightTestFixture, hashing_based_correct_backedge_weights_csr_representation) { | |
| std::string name = std::get<0>(GetParam()); | |
| GeneratorFunc generate = std::get<1>(GetParam()); | |
| const SInt n = 1000; | |
| const SInt m = 16 * n; | |
| const WeightRange weight_range{1, 100}; | |
| kagen::KaGen generator(MPI_COMM_WORLD); | |
| generator.UseCSRRepresentation(); | |
| generator.ConfigureEdgeWeightGeneration( | |
| kagen::EdgeWeightGeneratorType::HASHING_BASED, weight_range.first, weight_range.second); | |
| GeneratorFunc generate = std::get<1>(GetParam()); | |
| const SInt n = 1000; | |
| const SInt m = 16 * n; | |
| kagen::KaGen generator(MPI_COMM_WORLD); | |
| generator.UseEdgeListRepresentation(); | |
| generator.ConfigureEdgeWeightGeneration( | |
| kagen::EdgeWeightGeneratorType::HASHING_BASED, 1, 100); | |
| Graph graph = generate(generator, n, m); | |
| check_backedge_weight(graph); | |
| } | |
| TEST_P(GeneralEdgeWeightTestFixture, hashing_based_correct_backedge_weights_csr_representation) { | |
| GeneratorFunc generate = std::get<1>(GetParam()); | |
| const SInt n = 1000; | |
| const SInt m = 16 * n; | |
| kagen::KaGen generator(MPI_COMM_WORLD); | |
| generator.UseCSRRepresentation(); | |
| generator.ConfigureEdgeWeightGeneration( | |
| kagen::EdgeWeightGeneratorType::HASHING_BASED, 1, 100); |
| // ((void)u); | ||
| // ((void)v); | ||
| // throw std::runtime_error("xxHash is required for hashing based edge weights"); | ||
| return -1; |
Copilot
AI
Nov 7, 2025
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.
Commented-out code should be removed. The previous error-throwing behavior has been replaced with returning -1, which could mask configuration errors. If xxHash is truly optional, consider documenting this behavior change or using a different sentinel value that won't be confused with valid weights.
| // ((void)u); | |
| // ((void)v); | |
| // throw std::runtime_error("xxHash is required for hashing based edge weights"); | |
| return -1; | |
| throw std::runtime_error("xxHash is required for hashing based edge weights"); |
| } | ||
|
|
||
| TEST_P(GeneralEdgeWeightTestFixture, hashing_based_weights_in_range_edgelist_representation) { | ||
| std::string name = std::get<0>(GetParam()); |
Copilot
AI
Nov 7, 2025
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.
The variable name is extracted from test parameters but never used in the test body. Consider removing it to avoid confusion.
| } | ||
|
|
||
| TEST_P(GeneralEdgeWeightTestFixture, hashing_based_weights_in_range_csr_representation) { | ||
| std::string name = std::get<0>(GetParam()); |
Copilot
AI
Nov 7, 2025
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.
The variable name is extracted from test parameters but never used in the test body. Consider removing it to avoid confusion.
No description provided.