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.

Sign up for GitHub

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

Jump to bottom

Add the test for TakeDatasetOp #26632

Merged
merged 2 commits into from Mar 13, 2019
Merged

Add the test for TakeDatasetOp #26632

merged 2 commits into from Mar 13, 2019

Conversation

feihugis
Copy link
Member

This PR adds the test for TakeDatasetOp.

cc: @jsimsa @rachellim

jsimsa
jsimsa requested changes Mar 13, 2019
View reviewed changes
Copy link
Contributor

@jsimsa jsimsa left a comment

Choose a reason for hiding this comment

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

A couple of minor comments, but overall this looks good to me.

tensorflow/core/kernels/data/take_dataset_op_test.cc Outdated
/*breakpoints*/ {0, 2, 5, 11}};
}

class ParameterizedDatasetTest
Copy link
Contributor

Choose a reason for hiding this comment

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

ParametrizedTakeDatasetOpTest

tensorflow/core/kernels/data/take_dataset_op_test.cc Outdated
};

// Test case 1: take fewer than input size
TestCase TakeFewerTestCase() {
Copy link
Contributor

Choose a reason for hiding this comment

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

TakeLessTestCase

@jsimsa
Copy link
Contributor

jsimsa commented Mar 13, 2019

@feihugis Thank you for another unit test! I like that unit test style that we have converged on (defining test cases through functions that return a TestCase). When you get a chance, could you please apply it consistently across the existing unit tests? For instance, I recently made changes to tensor_slice_dataset_op_test.cc and sparse_tensor_slice_dataset_op_test.cc to fix an internal test failure and it would be great if you could apply your test style to those for consistency. Thanks again for the contributions!

@feihugis
Copy link
Member Author

@jsimsa Thanks for your quick review! The class and function names have been updated in this commit.

Sure, I will update the existing unit tests this week and keep the test style on the future PRs.

@rthadur rthadur self-assigned this Mar 13, 2019
@rthadur rthadur added this to Assigned Reviewer in PR Queue via automation Mar 13, 2019
@rthadur rthadur moved this from Assigned Reviewer to Approved by Reviewer in PR Queue Mar 13, 2019
@rthadur rthadur added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process size:L CL Change Size: Large labels Mar 13, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 13, 2019
@tensorflow-copybara tensorflow-copybara merged commit 66b8ba4 into tensorflow:master Mar 13, 2019
PR Queue automation moved this from Approved by Reviewer to Merged Mar 13, 2019
tensorflow-copybara pushed a commit that referenced this pull request Mar 13, 2019
@tensorflower-gardener
Merge pull request #26632 from feihugis:Test_TakeDataset
de2bea7 
PiperOrigin-RevId: 238291593
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsimsa jsimsa jsimsa approved these changes

Assignees

@rthadur rthadur

Labels
cla: yes ready to pull PR ready for merge process size:L CL Change Size: Large
Projects
PR Queue
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@feihugis @jsimsa @googlebot @kokoro-team @tensorflow-copybara @rthadur