Fixing tiny information loss in shuffling #607
Merged
Conversation
This file contains 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
…may be divisible by x, but the individual snapshot is not
|
This PR only tackles the numpy side of things, for OpenPMD it omits some of the logic. I will address this separately and have disabled one of the tests for now, |
|
LGTM. Checked it for shuffling input snapshots with different grid sizes and for a range of requested snapshot numbers |
timcallow
approved these changes
Nov 19, 2024
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.
As detailed by @timcallow in #564, our current shuffling can lead to information loss. This occurs when one requests a number of snapshots to be shuffled such that the total number of data points may be divisible by the new number of snapshots, while the individual number of grid points per snapshot are not.
This PR fixes this behavior by making the behavior implemented in #570 the default. I.e., snapshot shuffling now means:
(1. If the user does not provide a number for the snapshot files to be shuffled into, it is implicitly assumed that
number_after_shuffling = number_before_shuffling)2. It is checked whether the individual snapshot grid can be divided by this new number of snapshots
3. If not, the overall data size is reduced, and the algorithm implemented in #570 kicks in, discarding a tiny amount of information
4. Data is shuffled in snapshot-like files with dimensions (x,1,1,feature), i.e., effectively a 1D-array compatible with the MALA workflow.
This PR also fixes an issue with non-cubic cells and adds a test.