Fixing HeaderIterDP's __len__ function #166
Conversation
[ghstack-poisoned]
facebook-github-bot
added
the
CLA Signed
| self.length = min(source_len, self.limit) | ||
| return self.length | ||
| except TypeError: | ||
| warn( |
There was a problem hiding this comment.
Is raising a warning and returning the best guess (i.e. self.limit) the right decision here? It can be triggered by things like list(header_dp) if header_dp hasn't been fully traversed once yet, which might confuse the users. The alternative is to raise an exception and not return anything.
| # TODO(123): __len__ Test: returns the length of source when it is less than the limit | ||
| # header_dp = source_dp.header(30) | ||
| # self.assertEqual(20, len(header_dp)) | ||
| # __len__ Test: returns the length of source when it is less than the limit |
There was a problem hiding this comment.
These four tests here show what behaviors are expected
|
|
||
| def __iter__(self) -> Iterator[T_co]: | ||
| for i, value in enumerate(self.source_datapipe): | ||
| if i < self.limit: | ||
| yield value | ||
| else: | ||
| break | ||
| self.length = min(i + 1, self.limit) # We know length with certainty when we reach here |
There was a problem hiding this comment.
i + 1 for the case when source_datapipe has a length smaller than the limit, so the loop exits with i = len(source_datapipe) - 1.
There was a problem hiding this comment.
There is a minor bug that i would be unavailable if self.source_datapipe is empty. Let's declare a variable before the iteration to represent i.
There was a problem hiding this comment.
Could we do it in a pythonic way
| self.length = min(i + 1, self.limit) # We know length with certainty when we reach here | |
| i = 0 | |
| for value in self.source_datapipe: | |
| i += 1 | |
| if i <= self.limit: | |
| yield value | |
| else: | |
| break | |
| self.length = min(i, self.limit) |
|
|
||
| def __iter__(self) -> Iterator[T_co]: | ||
| for i, value in enumerate(self.source_datapipe): | ||
| if i < self.limit: | ||
| yield value | ||
| else: | ||
| break | ||
| self.length = min(i + 1, self.limit) # We know length with certainty when we reach here |
There was a problem hiding this comment.
There is a minor bug that i would be unavailable if self.source_datapipe is empty. Let's declare a variable before the iteration to represent i.
| self.length = min(source_len, self.limit) | ||
| return self.length | ||
| except TypeError: | ||
| warn( |
|
@NivekT has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
The previous implementation simply return the limit as the length for `HeaderIterDataPipe`. This updated implementation takes it a step further to account for the different possible scenarios. Fixes #123 Fixes #134 Differential Revision: [D33589168](https://our.internmc.facebook.com/intern/diff/D33589168) [ghstack-poisoned]
|
@NivekT has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Stack from ghstack:
The previous implementation simply return the limit as the length for
HeaderIterDataPipe. This updated implementation takes it a step further to account for the different possible scenarios.Fixes #123
Fixes #134
Differential Revision: D33589168