Skip to content

Conversation

@pvaneck
Copy link
Contributor

@pvaneck pvaneck commented Mar 9, 2020

This PR fixes fixes the epochs docstring for fitDataset to match what was changed here: 4393140

Some other minor fixes were added as well.


This change is Reviewable

@pvaneck pvaneck force-pushed the tf-data-fix branch 2 times, most recently from 779c61a to 7e2ec90 Compare June 2, 2020 22:41
Copy link
Contributor

@tafsiri tafsiri left a comment

Choose a reason for hiding this comment

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

Thanks for doing this (the little fixes do help!). I just have one request about the where to locate the information about the relationship between initialEpoch and epochs.

Reviewed 10 of 10 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @pvaneck)


tfjs-layers/src/engine/training_dataset.ts, line 42 at r1 (raw file):

  /**
   * The number of times to iterate over the training dataset.
   * Note that when used with `initialEpoch`, epochs is the index of the

I think this elaboration might better belong to the docs for initialEpochs to help guide the use of that param. This makes it so that you only think about this when you start to use intialEpochs.

Could you change that here and in the file referenced in your other commit.

Copy link
Contributor Author

@pvaneck pvaneck left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained (waiting on @tafsiri)


tfjs-layers/src/engine/training_dataset.ts, line 42 at r1 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

I think this elaboration might better belong to the docs for initialEpochs to help guide the use of that param. This makes it so that you only think about this when you start to use intialEpochs.

Could you change that here and in the file referenced in your other commit.

Good idea. Updated!

Copy link
Contributor

@tafsiri tafsiri left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained

@tafsiri
Copy link
Contributor

tafsiri commented Jun 4, 2020

Thanks!

@tafsiri tafsiri changed the title Add misc doc fixes Improve docs for initialEpochs Jun 4, 2020
@tafsiri tafsiri merged commit 550e2c7 into tensorflow:master Jun 4, 2020
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