8000 Remove bucket iterator shuffle warning. by HarshTrivedi · Pull Request #1742 · allenai/allennlp · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Dec 16, 2022. It is now read-only.

Remove bucket iterator shuffle warning. #1742

Merged
merged 3 commits into from
Sep 10, 2018

Conversation

HarshTrivedi
Copy link
Contributor

From discussion here: #1578. Let me know if I should remove the code instead of commenting it.

From discussion here: allenai#1578. Let me know if I should remove the code instead of commenting it.
else:
logger.warning("shuffle parameter is set to False,"
" while bucket iterators by definition change the order of your data.")
# else:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this should be a comment in the code inside the if shuffle block, like # NOTE: if shuffle is false, the data will still be in a different order because of the bucket sorting. We generally don't allow commented-out code to be checked in, because it's not obvious why the code is commented out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. So I believe that you mean to remove the else block code and replace the if block by the following:

            if shuffle:
                # NOTE: if shuffle is false, the data will still be in a different order
                # because of the bucket sorting
                random.shuffle(batches)

right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly.

Copy link
Contributor
@matt-gardner matt-gardner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM!

@matt-gardner matt-gardner merged commit 72f7b4b into allenai:master Sep 10, 2018
@HarshTrivedi HarshTrivedi deleted the patch-1 branch February 3, 2019 01:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0