Fix bug due to rounding errors in periodicity computation#146
Draft
efaulhaber wants to merge 1 commit intomainfrom
Draft
Fix bug due to rounding errors in periodicity computation#146efaulhaber wants to merge 1 commit intomainfrom
efaulhaber wants to merge 1 commit intomainfrom
Conversation
Contributor
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.
Comment on lines
+609
to
613
| @inline function nonperiodic_cell_coords(coords, neighborhood_search) | ||
| (; cell_size) = neighborhood_search | ||
|
|
||
| @inline function cell_coords(coords, periodic_box::PeriodicBox, cell_list, cell_size) | ||
| # Subtract `min_corner` to offset coordinates so that the min corner of the periodic | ||
| # box corresponds to the (0, 0, 0) cell of the NHS. | ||
| # This way, there are no partial cells in the domain if the domain size is an integer | ||
| # multiple of the cell size (which is required, see the constructor). | ||
| offset_coords = periodic_coords(coords, periodic_box) .- periodic_box.min_corner | ||
|
|
||
| # Add one for 1-based indexing. The min corner will be the (1, 1, 1)-cell. | ||
| return Tuple(floor_to_int.(offset_coords ./ cell_size)) .+ 1 | ||
| return Tuple(floor_to_int.(coords ./ cell_size)) | ||
| end |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.