Skip to content
This repository was archived by the owner on Dec 16, 2022. It is now read-only.

Turn off shuffling during evaluation #1578

Merged
merged 2 commits into from
Aug 6, 2018

Conversation

nelson-liu
Copy link
Contributor

No description provided.

@nelson-liu nelson-liu requested a review from joelgrus August 6, 2018 18:07
@nelson-liu nelson-liu merged commit 6b37dd2 into allenai:master Aug 6, 2018
@nelson-liu nelson-liu deleted the turn_off_evaluation_shuffling branch August 6, 2018 18:35
gabrielStanovsky pushed a commit to gabrielStanovsky/allennlp that referenced this pull request Sep 7, 2018
@HarshTrivedi
Copy link
Contributor

@nelson-liu I keep getting this warning during validation evaluation after each epoch training.

allennlp.data.iterators.bucket_iterator - shuffle parameter is set to False, while bucket iterators by definition change the order of your data.

Since shuffling for validation has been explicitly turned off, should warning for validation iterator be turned off ?

@nelson-liu
Copy link
Contributor Author

I don't have a strong opinion either way, but you're right that the message isn't too useful now that validation shuffle is always false.

@matt-gardner
Copy link
Contributor

I think it's probably fine to move that warning to a comment in the code.

@HarshTrivedi
Copy link
Contributor

@matt-gardner That would cause the warning to not show for train iterator as well. Would that be fine?

@matt-gardner
Copy link
Contributor

I think so. The probability of running into the issue that's being warned about is incredibly small, so we should optimize for the much more common case.

HarshTrivedi added a commit to HarshTrivedi/allennlp that referenced this pull request Sep 10, 2018
From discussion here: allenai#1578. Let me know if I should remove the code instead of commenting it.
matt-gardner pushed a commit that referenced this pull request Sep 10, 2018
* Remove bucket iterator shuffle warning.

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

* Remove the warning and add comment instead.

* Add a period.
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.

4 participants