Skip to content
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

Raise error if len(DynamicBatchSampler()) is ambiguous #8137

Merged
merged 5 commits into from
Oct 6, 2023

Conversation

akihironitta
Copy link
Member

Part of a fix for #7724.

Assuming len(DynamicBatchSampler(dataset)) == len(dataset) is wrong in AFAIK all cases max_num>1, which can lead to undesired behaviour in 3rd party libraries like PyTorch Lightning as reported in the linked issue.

@codecov
Copy link

codecov bot commented Oct 6, 2023

Codecov Report

Merging #8137 (130f42a) into master (aae8dfd) will decrease coverage by 0.70%.
The diff coverage is 100.00%.

❗ Current head 130f42a differs from pull request most recent head 29b3a92. Consider uploading reports for the commit 29b3a92 to get more accurate results

@@            Coverage Diff             @@
##           master    #8137      +/-   ##
==========================================
- Coverage   88.84%   88.14%   -0.70%     
==========================================
  Files         471      471              
  Lines       28278    28275       -3     
==========================================
- Hits        25123    24923     -200     
- Misses       3155     3352     +197     
Files Coverage Δ
torch_geometric/loader/dynamic_batch_sampler.py 95.65% <100.00%> (+2.31%) ⬆️

... and 34 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@akihironitta akihironitta marked this pull request as ready for review October 6, 2023 05:46
@akihironitta akihironitta requested review from wsad1, mananshah99 and a team as code owners October 6, 2023 05:46
@github-actions github-actions bot removed the sampler label Oct 6, 2023
@akihironitta akihironitta changed the title Don't define DynamicBatchSampler.__len__ Raise error if len(DynamicBatchSampler()) is ambiguous Oct 6, 2023
@rusty1s rusty1s enabled auto-merge (squash) October 6, 2023 07:06
@rusty1s rusty1s merged commit 412206b into master Oct 6, 2023
@rusty1s rusty1s deleted the dynamic_batch_sampler_len branch October 6, 2023 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants