Clamp probabilities passed to binomial distribution to $[0.0, 1.0]$. #86
Conversation
When the probability is computed based on chunk and cell sizes which are equal, floating point inaccuracies may lead to probabilities greater than 1.0.
There was a problem hiding this comment.
Pull request overview
This PR addresses floating point inaccuracy issues by clamping probability values passed to binomial distributions to the valid range [0.0, 1.0]. When chunk and cell sizes are equal, floating point arithmetic can produce probabilities slightly greater than 1.0, which would be invalid for binomial distributions.
Key Changes:
- Added
std::clampcalls to ensure probabilities remain within [0.0, 1.0] - Added
<algorithm>header includes where needed forstd::clamp - Applied the fix consistently across hyperbolic and geometric (2D/3D) graph generators
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| kagen/generators/hyperbolic/hyperbolic.cpp | Clamped probabilities in annuli and cell generation with proper bounds |
| kagen/generators/geometric/rgg/rgg_3d.cpp | Clamped probability in 3D cell generation with proper bounds |
| kagen/generators/geometric/rgg/rgg_2d.cpp | Critical bug: Used incorrect upper bound of 0.1 instead of 1.0 |
| kagen/generators/geometric/geometric_3d.h | Clamped probability in 3D geometric base class with proper bounds |
| kagen/generators/geometric/geometric_2d.h | Critical bugs: Syntax error with std::cell_area and inconsistent indentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| SInt h = sampling::Spooky::hash(seed); | ||
| SInt cell_vertices = rng_.GenerateBinomial(h, n, cell_area / total_area); | ||
| // due to potential floating point inaccuracies clamp probability | ||
| SInt cell_vertices = rng_.GenerateBinomial(h, n, std::clamp(cell_area / total_area, 0.0, 1.0)); |
There was a problem hiding this comment.
[nitpick] For type safety and consistency, consider using explicit type casting for the clamp bounds. Since cell_area and total_area are of type LPFloat, use std::clamp(cell_area / total_area, LPFloat{0.0}, LPFloat{1.0}) to match the pattern used in hyperbolic.cpp with Double{0.0} and Double{1.0}.
| SInt cell_vertices = rng_.GenerateBinomial(h, n, std::clamp(cell_area / total_area, 0.0, 1.0)); | |
| SInt cell_vertices = rng_.GenerateBinomial(h, n, std::clamp(cell_area / total_area, LPFloat{0.0}, LPFloat{1.0})); |
| seed = config_.seed + chunk_id * cells_per_chunk_ + i + total_chunks_ * cells_per_chunk_; | ||
| SInt h = sampling::Spooky::hash(seed); | ||
| SInt cell_vertices = rng_.GenerateBinomial(h, n, cell_area / total_area); | ||
| SInt cell_vertices = rng_.GenerateBinomial(h, n, std::clamp(cell_area / total_area, 0.0, 1.0)); |
There was a problem hiding this comment.
[nitpick] For type safety and consistency, consider using explicit type casting for the clamp bounds. Since cell_area and total_area are of type LPFloat, use std::clamp(cell_area / total_area, LPFloat{0.0}, LPFloat{1.0}) to match the pattern used in hyperbolic.cpp with Double{0.0} and Double{1.0}.
| SInt cell_vertices = rng_.GenerateBinomial(h, n, std::clamp(cell_area / total_area, 0.0, 1.0)); | |
| SInt cell_vertices = rng_.GenerateBinomial(h, n, std::clamp(cell_area / total_area, LPFloat{0.0}, LPFloat{1.0})); |
| SInt h = sampling::Spooky::hash(seed); | ||
| SInt cell_vertices = rng_.GenerateBinomial(h, n, cell_area / total_area); | ||
| // due to potential floating point inaccuracies clamp probability | ||
| SInt cell_vertices = rng_.GenerateBinomial(h, n, std::clamp(cell_area / total_area, 0.0, 1.0)); |
There was a problem hiding this comment.
[nitpick] For type safety and consistency, consider using explicit type casting for the clamp bounds. Since cell_area and total_area are of type LPFloat, use std::clamp(cell_area / total_area, LPFloat{0.0}, LPFloat{1.0}) to match the pattern used in hyperbolic.cpp with Double{0.0} and Double{1.0}.
| SInt cell_vertices = rng_.GenerateBinomial(h, n, std::clamp(cell_area / total_area, 0.0, 1.0)); | |
| SInt cell_vertices = rng_.GenerateBinomial(h, n, std::clamp(cell_area / total_area, LPFloat{0.0}, LPFloat{1.0})); |
| // Variate | ||
| if (!clique) | ||
| seed = config_.seed + annulus_id * config_.k + chunk_id + i + config_.n; | ||
| SInt h = sampling::Spooky::hash(seed); |
There was a problem hiding this comment.
[nitpick] Missing explanatory comment. For consistency with line 192-193 and the other files in this PR, add a comment here: // due to potential floating point inaccuracies clamp probability
| SInt h = sampling::Spooky::hash(seed); | |
| SInt h = sampling::Spooky::hash(seed); | |
| // due to potential floating point inaccuracies clamp probability |
| seed = config_.seed + annulus_id * config_.k + chunk_id + i + config_.n; | ||
| SInt h = sampling::Spooky::hash(seed); | ||
| SInt n_cell = rng_.GenerateBinomial(h, n, grid_phi / total_phi); | ||
| SInt n_cell = rng_.GenerateBinomial(h, n, std::clamp(grid_phi / total_phi, Double{0.0}, Double{1.0})); |
There was a problem hiding this comment.
[nitpick] Inconsistent spacing in type construction. Line 193 uses Double {0.0} (with space), but this line uses Double{0.0} (without space). For consistency, use the same spacing convention as line 193.
| SInt n_cell = rng_.GenerateBinomial(h, n, std::clamp(grid_phi / total_phi, Double{0.0}, Double{1.0})); | |
| SInt n_cell = rng_.GenerateBinomial(h, n, std::clamp(grid_phi / total_phi, Double {0.0}, Double {1.0})); |
When the probability is computed based on chunk and cell sizes which
are equal, floating point inaccuracies may lead to probabilities
greater than 1.0.
This closes #85