gh #7131 Support for astype with literal strings by njriasan · Pull Request #7132 · numba/numba

njriasan · 2021-06-21T22:32:57Z

Adds support for Array.astype with string literal type names. This required two changes:

Adding a lower_builtin for string literals.
Fixes a bug in get_call_type where the literal option would never be tried if the unliteral version was preferred and returned None.

Reference an existing issue

esc · 2021-06-22T08:59:35Z

@njriasan thank you for submitting this to the Numba issue tracker. I have added it to the queue for review. Please note that we are currently in a burn-down period towards a release candidate, so the review for this PR may be somewhat delayed. We thank you in advance for your patience and understanding!

stuartarchibald

Thanks for the patch, couple of things to resolve else looks good.

njriasan · 2021-07-09T20:38:21Z

@stuartarchibald I've update this PR with your suggested changes. Thank you.

stuartarchibald

Thanks for the update, couple of minor things to resolve else looks good.

njriasan · 2021-09-09T03:40:48Z

@stuartarchibald Sorry for the delay. I finally got the time to finalize this PR.

njriasan · 2021-09-24T00:30:31Z

@stuartarchibald Just wanted to send you another ping in case the notification got lost in your inbox. No rush on this, but it should be a quick review when you have time.

stuartarchibald

Thanks for the update, I've left some comments inline, I think given the nature of the corner cases being tested being very explicit in the tests is preferable. Thanks again!

stuartarchibald · 2021-10-05T14:14:43Z

+            cres = self.ccache.compile(pyfunc, (typeof(arr), typeof(dtype)))
+            return cres.entry_point(arr, dtype)


This is not reached in the current tests.

stuartarchibald · 2021-10-05T14:15:09Z

+            """
+            expected = arr.astype(dtype).copy(order='A')
+            got = run_dtype_arg(arr, dtype)
+            self.assertPreciseEqual(got, expected)


This is not reached in the current tests.

stuartarchibald · 2021-10-05T14:20:10Z

+        # Non-Literal String
+        unicode_val = "float32"
+        with self.assertTypingError() as raises:
+            check_dtype_arg(arr, unicode_val)
+        self.assertIn('array.astype if dtype is a string it must be constant',
+                      str(raises.exception))


Suggest something like this as it's really only this one case that exists?:

diff --git a/numba/tests/test_array_methods.py b/numba/tests/test_array_methods.py index 64c0eba..c130d88 100644 --- a/numba/tests/test_array_methods.py +++ b/numba/tests/test_array_methods.py @@ -469,25 +469,6 @@ class TestArrayMethods(MemoryLeakMixin, TestCase): got = run(arr, dtype) self.assertPreciseEqual(got, expected) - def run_dtype_arg(arr, dtype): - """ - Equivalent to run but dtype is passed as - an arg instead of as a lowered constant. - This is used to check LiteralValue requirements. - """ - pyfunc = lambda arr, dtype: arr.astype(dtype) - cres = self.ccache.compile(pyfunc, (typeof(arr), typeof(dtype))) - return cres.entry_point(arr, dtype) - def check_dtype_arg(arr, dtype): - """ - Equivalent to check but dtype is passed as - an arg instead of as a lowered constant. - This is used to check LiteralValue requirements. - """ - expected = arr.astype(dtype).copy(order='A') - got = run_dtype_arg(arr, dtype) - self.assertPreciseEqual(got, expected) - # C-contiguous arr = np.arange(24, dtype=np.int8) check(arr, np.dtype('int16')) @@ -515,10 +496,14 @@ class TestArrayMethods(MemoryLeakMixin, TestCase): check(arr, dt) self.assertIn('cannot convert from int32 to Record', str(raises.exception)) - # Non-Literal String + + # Check non-Literal string raises unicode_val = "float32" with self.assertTypingError() as raises: - check_dtype_arg(arr, unicode_val) + @jit(nopython=True) + def foo(dtype): + np.array([1]).astype(dtype) + foo(unicode_val) self.assertIn('array.astype if dtype is a string it must be constant', str(raises.exception))

stuartarchibald · 2021-10-05T14:24:31Z

        assert not kws
        dtype, = args
+        if dtype == types.unicode_type:
+            raise RequireLiteralValue("array.astype if dtype is a string it must be constant")


This line needs wrapping to 80 chars.

stuartarchibald · 2021-10-05T14:24:34Z

        from .npydecl import parse_dtype
        assert not kws
        dtype, = args
+        if dtype == types.unicode_type:


Suggested change

if dtype == types.unicode_type:

if isinstance(dtype, types.UnicodeType):

stuartarchibald

This should fix CI.

Fix CI issue. Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>

njriasan · 2021-10-05T17:35:30Z

This should fix CI.

Thank you!

njriasan · 2021-10-05T20:48:40Z

@stuartarchibald I made all of your suggested changes. Let me know if this PR needs anything else?

stuartarchibald

Thanks for the patch and fixes.

stuartarchibald · 2021-10-06T09:03:49Z

@njriasan Congratulations on your first contribution to Numba!

gh numba#7131 support for astype with literal strings

7abe006

njriasan requested review from esc, sklam and stuartarchibald as code owners June 21, 2021 22:32

esc added the 3 - Ready for Review label Jun 22, 2021

stuartarchibald reviewed Jul 7, 2021

View reviewed changes

Comment thread numba/core/types/functions.py

Comment thread numba/core/types/functions.py Outdated

stuartarchibald added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Jul 7, 2021

stuartarchibald added this to the Numba 0.55 RC milestone Jul 7, 2021

stuartarchibald added the Effort - short Short size effort needed label Jul 7, 2021

Nicholas J Riasanovsky added 3 commits July 9, 2021 13:27

numba/core/types/functions.py

7e86ad9

updated arraydecl to throw require literal value

1646701

reverted function changes

a8e24a9

njriasan requested a review from stuartarchibald July 12, 2021 01:36

stuartarchibald reviewed Aug 3, 2021

View reviewed changes

Comment thread numba/core/typing/arraydecl.py Outdated

Comment thread numba/core/typing/arraydecl.py Outdated

Nicholas J Riasanovsky added 2 commits August 3, 2021 11:23

fixed import issue

1551c2a

Added a test that unicode value must be a string literal

82062a8

njriasan requested a review from stuartarchibald September 9, 2021 03:40

stuartarchibald reviewed Oct 5, 2021

View reviewed changes

Nicholas J Riasanovsky added 2 commits October 5, 2021 12:36

Made Stuart's changes

2c8fa0a

fixed typo

f7665e4

stuartarchibald reviewed Oct 5, 2021

View reviewed changes

Comment thread numba/core/typing/arraydecl.py Outdated

Update numba/core/typing/arraydecl.py

97a54df

Fix CI issue. Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>

stuartarchibald approved these changes Oct 6, 2021

View reviewed changes

stuartarchibald added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on author Waiting for author to respond to review labels Oct 6, 2021

sklam merged commit efb0288 into numba:master Oct 6, 2021

		cres = self.ccache.compile(pyfunc, (typeof(arr), typeof(dtype)))
		return cres.entry_point(arr, dtype)

	if dtype == types.unicode_type:
	if isinstance(dtype, types.UnicodeType):

Conversation

njriasan commented Jun 21, 2021

Reference an existing issue

Uh oh!

esc commented Jun 22, 2021

Uh oh!

stuartarchibald left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

njriasan commented Jul 9, 2021

Uh oh!

stuartarchibald left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

njriasan commented Sep 9, 2021

Uh oh!

njriasan commented Sep 24, 2021

Uh oh!

stuartarchibald left a comment

Choose a reason for hiding this comment

Uh oh!

stuartarchibald Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

stuartarchibald Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

stuartarchibald Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stuartarchibald Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

stuartarchibald Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

stuartarchibald left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

njriasan commented Oct 5, 2021

Uh oh!

njriasan commented Oct 5, 2021

Uh oh!

stuartarchibald left a comment

Choose a reason for hiding this comment

Uh oh!

stuartarchibald commented Oct 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants