Fix numba.typed.List extend for singleton and empty iterable #5517
Conversation
Fixes numba#5152 An implicit assumption was being made about the length of the iterable when refining with extend. Specifically, this assumtion was that the iterable has lenth >= 2. The code would however fail if the length is 1, so this commit fixes that. Tests included.
This allows an untyped List to be extended with an empty iterable in the interpreter. This is results in a NO-OP and is probably unlikely to occur often in practice but is fixed for completeness. Tests included.
stuartarchibald
changed the title
A `numba.typed.List` that is refined failed to extend on an empty iterable. This fixes that to be a NO-OP and includes tests.
numba/tests/test_typedlist.py
Outdated
| @@ -729,6 +729,33 @@ def impl(): | |||
| got = impl() | |||
| self.assertEqual(expected, got) | |||
|
|
|||
| def test_extend_singleton(self): | |||
There was a problem hiding this comment.
Where's the singleton? Do you mean, "container holding single value"?
There was a problem hiding this comment.
yes, I was under the impression, that the term 'singleton' could be used to describe such a construct, but perhaps I was mistaken. Is there something shorter than "container holding single value"?
There was a problem hiding this comment.
It seems, overwhelmingly, singleton refers to the singleton pattern, with the exception of tuple, https://docs.python.org/3/library/stdtypes.html#tuple.
There was a problem hiding this comment.
Thanks for looking into this. So it seems that use of the term 'singleton' in this context is perhaps permissible. In any case to avoid any future ambiguities I think it will be best to rename the test, how about 'single_value_container'?
There was a problem hiding this comment.
Sounds good, thanks.
esc
added
bug
4 - Waiting on reviewer
stuartarchibald
added
4 - Waiting on author
The 'term' singleton may not have been the appropriate choice here.
esc
added
4 - Waiting on reviewer
|
@stuartarchibald this one is fixed up too and ready for the next round of reviews. |
| # Extending an unrefined list with an empty iterable doesn't work in a | ||
| # jit compiled function as the list remains untyped. | ||
| l = List() | ||
| l.extend(tuple()) | ||
| self.assertEqual(len(l), 0) |
There was a problem hiding this comment.
Is this behaviour a good idea? What about JIT transparency etc.
There was a problem hiding this comment.
Also, if you put this in a JIT compiled function, you get:
Invalid use of Function(<function impl_extend at 0x7f5648715c10>) with argument(s) of type(s): (ListType[undefined], Tuple())
* parameterized
In definition 0:
TypingError: extend argument must be iterable
empty tuples are iterable, this is a bit confusing. I think it's arising because an empty tuple is typed as Tuple at present, which is not iterable for Numba's purposes.
There was a problem hiding this comment.
Yeah, I think only UniTuple is iterable. I struggled with this one for some time and came to the conclusion that this is a corner case that only works outside of a jit context.
There was a problem hiding this comment.
So, even if tuple() would be something iterable, the function would not compile as the list can not be refined.
numba/typed/typedlist.py
Outdated
| self.append(iterable[0]) | ||
| return _extend(self, iterable[1:]) | ||
| return _extend(self, iterable) | ||
| # Iterable has more values to extend from | ||
| if iterable_length > 1: | ||
| return _extend(self, iterable[1:]) | ||
| else: | ||
| return self |
There was a problem hiding this comment.
Could this just be:
self._initialise_list(iterable[0])
return _extend(self, iterable)
i.e. init type, then extend, is there a need to append then slice from 1?
There was a problem hiding this comment.
Yes, this can be significantly simplified.
This simplifies the logic needed to implement `extend`.
stuartarchibald
added
5 - Ready to merge
|
@stuartarchibald awesome, thanks for the review! |
|
thanks for merge |
Fixes #5152