Fix bug due to rounding errors in periodicity computation#146
Fix bug due to rounding errors in periodicity computation#146efaulhaber wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a rounding error bug in periodicity computation by changing the approach: instead of first wrapping coordinates into the periodic box with periodic_coords and then computing cell indices (which can suffer from floating-point rounding at boundaries), it now computes cell indices from raw coordinates and then applies modular arithmetic to handle periodicity.
Changes:
- Unified
cell_coordsandperiodic_cell_indexinto generic versions that no longer dispatch on cell list type, usingmod(. - 2, n_cells) + 2for periodic wrapping - Removed
FullGridCellList-specific specializations ofcell_coordsandperiodic_cell_indexfromfull_grid.jl - Introduced
nonperiodic_cell_coordsas a separate step before periodic index wrapping
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/nhs_grid.jl | Refactored cell_coords to separate non-periodic coordinate computation from periodic wrapping; unified periodic_cell_index to use 2-based modulo for all cell list types |
| src/cell_lists/full_grid.jl | Removed FullGridCellList-specific cell_coords and periodic_cell_index specializations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #146 +/- ##
==========================================
- Coverage 86.06% 85.40% -0.66%
==========================================
Files 15 15
Lines 739 733 -6
==========================================
- Hits 636 626 -10
- Misses 103 107 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
efaulhaber
left a comment
There was a problem hiding this comment.
Codex Review
The PR appears intended to centralize periodic cell-index wrapping and make FullGridCellList use the same periodic path as other cell lists.
This is an AI-generated code review. Please verify the findings and summary before acting on them.
Review generated by codex-pr-review
On main, we first computed the periodic continuous coordinates and then converted to discrete cell coordinates. The first computation introduces rounding errors, sometimes resulting in coordinates outside the periodic box, which will then be converted to discrete cell coordinates outside the box as well. This leads to out-of-bounds errors.
This PR first computes the discrete cell coordinates and then applies a discrete modulo, so there are no rounding errors that can break anything.