Bug fix: Loader.__len__ returns wrong length#144
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #144 +/- ##
==========================================
- Coverage 93.19% 91.19% -2.01%
==========================================
Files 10 10
Lines 823 829 +6
==========================================
- Hits 767 756 -11
- Misses 56 73 +17
🚀 New features to boost your workflow:
|
ilan-gold
left a comment
ilan-gold
left a comment
There was a problem hiding this comment.
Can we add a test for this now? I would think one of the old torch tests were broken silently?
| ) | ||
| loader.add_dataset(**data) | ||
|
|
||
| expected_len = n_obs // batch_size if drop_last else math.ceil(n_obs / batch_size) |
There was a problem hiding this comment.
Put this as a parametrized number explicitly. Also not sure why we're parametrizing on chunk size or number of chunks to preload. Can you remove? Seems unrelated
There was a problem hiding this comment.
It's supposed to also check the n_iters function of the ChunkSampler. This covers different edge cases. I can remove it, if we only wanna check the n_iters.
There was a problem hiding this comment.
But https://github.com/scverse/annbatch/pull/144/changes#diff-102eacca529505a20b2ae79d187762c2125e058343b16a2452f981a82f7c17bcR100-R103 doesn't use those parameters - it's just batch_size and n_obs dependent along with the mask - so maybe parametrize by mask start/end?
There was a problem hiding this comment.
ok, so this rathe tests if the ChunkSampler handles this correctly. Thinking about this, this isn't really the point of this test. I've removed it now 👍
2 participants
In the current implementation
Loader.__len__returnsn_obsinstead to the number of batches. This PR should fix this