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

reshuffle_each_iteration args now default to True #16810

Closed
wants to merge 1 commit into from

Conversation

kr-ish
Copy link
Contributor

@kr-ish kr-ish commented Feb 6, 2018

The documentation for the tf.Dataset.data.shuffle function states the following:

  • reshuffle_each_iteration: (Optional.) A boolean, which if true indicates that the dataset should be pseudorandomly reshuffled each time it is iterated over. (Defaults to True.)

However, the default value in the function is None:

def shuffle(self, buffer_size, seed=None, reshuffle_each_iteration=None):

The function calls the ShuffleDataset class, whose __init__ function also sets the same argument to None by default, and uses the following logic to set the default value of the argument to True:

if reshuffle_each_iteration is None:
  self._reshuffle_each_iteration = True
else:
  self._reshuffle_each_iteration = reshuffle_each_iteration

This commit sets the argument to True by default in both the function and the class, making the above code block redundant and replacing it with only self._reshuffle_each_iteration = reshuffle_each_iteration.

reshuffle_each_iteration now defaults to True instead of defaulting to None and assigning self._reshuffle_each_iteration to True if value is None
@mrry
Copy link
Contributor

mrry commented Feb 7, 2018

Thanks for the contribution, but unfortunately I am not going to accept this PR for a few reasons:

  • It would be a breaking change for any existing code that calls Dataset.shuffle(..., reshuffle_each_iteration=None).
  • In general I prefer None for default arguments, because it makes it easier to write wrappers around an API. If the default argument is set to True, it's difficult for a wrapper to use the default of the wrapped function, without making a copy of the default at each layer of wrapping. Using None and implementing the default at the innermost level is simpler to maintain.
  • Using None is also consistent with the necessary style when the default argument can be a mutable type (like a list) and it is unsafe to put the default value in the argument list.

@mrry mrry closed this Feb 7, 2018
@kr-ish
Copy link
Contributor Author

kr-ish commented Feb 7, 2018

Thank you for reviewing this! I appreciate the details- using None makes sense to me now. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants