-
Notifications
You must be signed in to change notification settings - Fork 29
prevent empty or partial batches #281
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #281 +/- ##
==========================================
+ Coverage 96.25% 96.95% +0.70%
==========================================
Files 6 6
Lines 347 394 +47
Branches 82 59 -23
==========================================
+ Hits 334 382 +48
+ Misses 8 6 -2
- Partials 5 6 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The proposed change doesn't account for overlaps, I'll reopen once I get that part working. |
synflyn28
left a comment
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 code looks clean and readable to me. I have nothing else to add.
|
Ok, now this works well for overlaps and other wacky inputs. The resulting batches are always the same size. For example, the assertions here work: The test helper functions assumed that batch size was a multiple of input size, so I had to modify that to account for cases where |
|
Hmm, well those builds are failing because of running out of space, not anything in xbatcher failing. I still think this change is ready. |
Description of proposed changes
Fixes #184. In the current version batch dimensions that are not a multiple of input dimensions result in empty or partial batches. The empty batches cause an error when accessed. For example:
This occurs because
_gen_batch_numbersassumes the size of a batch on a dimension is equal to the entry inbatch_dims, which isn't the case whenbatch_size % input_size != 0and there is no overlap. This scenario wasn't in any of the tests, so I'm not sure what intended behavior is here. Imo this implies a partial patch and should raise an error. This PR adds a check thatbatch_size % input_size == 0for all duplicate dims, and raises the belowValueErrorif otherwise.