added slice + flatten for iterable datapipes #730
Conversation
facebook-github-bot
added
the
CLA Signed
dbish
changed the title
dbish
changed the title
dbish
changed the title
dbish
changed the title
There was a problem hiding this comment.
I chatted with Vitaly and understand what the intention is with the default behavior when nothing is passed in.
We also chatted about changing the name from flatten to flatten_sample but that is probably too much and potentially more confusing.
I would recommend:
- Being explicit in the docstring about the
flattenoperation is being applied at the element/sample level. - Adding one example in docstring with the default behavior when nothing is passed in. This can be copied over from one of the unit tests.
Otherwise, LGTM!
|
@dbish has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| slice_dp = input_dp.slice(0, 2) | ||
| self.assertEqual([[0, 1], [3, 4], [6, 7]], list(slice_dp)) | ||
|
|
||
| # Functional Test: filter with list of indices for list |
There was a problem hiding this comment.
Same as previous lines?
There was a problem hiding this comment.
Overall good.
As part of the exercise you can consider including assertWarns tests for borderline situations:
https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertWarns
| input_dp = IterableWrapper([["zero", ["one", "2"]], ["3", ["4", "5"]], ["6", ["7", "8"]]]) | ||
| flatten_dp = input_dp.flatten() | ||
| self.assertEqual([["zero", "one", "2"], ["3", "4", "5"], ["6", "7", "8"]], list(flatten_dp)) | ||
|
|
There was a problem hiding this comment.
Nit: Probably include test and mention in the doc, that we are not supporting nesting levels > 1
|
@dbish has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Please read through our contribution guide prior to
creating your pull request.
Fixes #656
Changes