-
Notifications
You must be signed in to change notification settings - Fork 19.5k
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
Updating split range from [0, 1] to (0, 1) in keras.utils.split_dataset #18028
Conversation
…taset to make it more intuitive In the split_dataset API which is used to split a dataset into left and right parts. But documentation states it splits dataset into left half and right half where the parts need not be equal parts.The split can be anything in range(0, 1).Hence I think to replace the word half with part. Also in the argument section and exception section the range for split used as [0, 1] which is not a correct notation as 0 and 1 are actually excluded in the range and raises exception if we use either 0 or 1 for either of left_size or right_size for splitting the dataset. Hence i have replaced [0, 1] with (0,1) to make it more intuitive. Thanks.
keras/utils/dataset_utils.py
Outdated
|
||
Args: | ||
dataset: A `tf.data.Dataset` object, or a list/tuple of arrays with the | ||
same length. | ||
left_size: If float (in the range `[0, 1]`), it signifies | ||
left_size: If float (in the range `(0, 1)`), it signifies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep the brackets for the range -- it mimics math notation for ranges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not getting your question. Actually the API raises error if split includes either 0
or 1
. As per maths notation
[0, 1]
means both 0
and 1
are included in the range. But here these are not valid values for argument. Hence I am proposing the range as (0, 1)
. Am I understand it correctly ?
Hi @SuryanarayanaY Can you please check @fchollet's comments ? Thank you! |
This PR is stale because it has been open for 14 days with no activity. It will be closed if no further activity occurs. Thank you. |
Hi @SuryanarayanaY Any update on this PR? Please. Thank you! |
Hi @SuryanarayanaY Any update on this PR? Please. Thank you! |
Added my comments above |
Hi @fchollet Can you please assist on above comments from @SuryanarayanaY. Thank you! |
The notation |
Updated the range notation as requested
keras/utils/dataset_utils.py
Outdated
|
||
Args: | ||
dataset: A `tf.data.Dataset` object, or a list/tuple of arrays with the | ||
same length. | ||
left_size: If float (in the range `[0, 1]`), it signifies | ||
left_size: If float (in the range "[0, 1] (excluding the boundary values 0 and 1)"), it signifies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- use backticks for the
[]
range - remove
"
quotes - make sure the lines are under 80 chars
Hi @SuryanarayanaY Can you please check @fchollet's comments and keep us posted ? Thank you! |
Don e the formatting changes as requested
Apologies for the delay. Done the changes as requested. Thanks |
keras/utils/dataset_utils.py
Outdated
left_size: If float (in the range `[0, 1]` (excluding the | ||
boundary values 0 and 1)), it signifies the fraction of the data to | ||
pack in the left dataset.If integer, it signifies the number of | ||
samples to pack in the left dataset.If `None`, it uses the complement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
Hi @SuryanarayanaY Can you please check @grasskin's comments and keep us posted ? Thank you! |
Done the formatting as suggested. Thanks
Apologies for the delay.Done the suggested changes. |
Removed trailing whitespaces as suggested by lint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Hello, Thank you for submitting a pull request. We're currently in the process of migrating the new |
In the
tf.keras.utils.split_dataset
API , that can be used to split a dataset into left and right parts, the documentation states it splits dataset intoleft half
andright half
where the parts need not be equal parts.The split can be anything in range (0, 1). Hence I thought to replace the wordhalf
withpart
.Also in the
Args
section of documentation andexception handling
insource code
the range for split used as[0, 1]
which is not a correct notation as 0 and 1 are actually not inclusive in the range and raises exception if we use either0 or 1
for either ofleft_size
orright_size
for splitting the dataset. Hence I have replaced[0, 1]
with(0, 1)
to make it more intuitive. Thanks.