String to int conversion

[Alexander-Makaryev]

This is the port of native implementation from cpython.

[stuartarchibald]

@Alexander-Makaryev thanks for this PR, is it ready for review? If not, please just comment on here when you'd like it reviewed and the core devs will schedule it. Thanks!

[Alexander-Makaryev]

@stuartarchibald yes, it is ready for review.
Another one (#3985) is ready too. It is done on the top of this branch and implementation is very close to this PR, so I think that it is a good idea to review both PRs by one developer at the same time.

[stuartarchibald]

@Alexander-Makaryev great, thanks for confirming, I'll schedule them for review.

[sklam]

(I have only reviewed the organization of the code and not much of the C code. )
Instead of creating a ext_string_conversion, I'd suggest adding the feature into _helperlib so that the new C functions are exposed via the c_helpers. For reference, you can look at how numba_dict_new_minsize is implemented in _dictobject.c and exposed in _helpermod.c (with declmethod(dict_new_minsize);). Lastly, dictobject.py creates an intrinsic to reference it via a .get_or_insert_function(fnty, name='numba_dict_new_minsize')

[sklam]

This is making a top-level module. It should be nested inside numba. i.e. name='numba._string_conversion_ext'. But better yet is to build numba/_string_conversion.c into numba._helperlib.

[sklam]

string_conversion_ext should not be a top-level module. See comment in setup.py

[sklam]

Please reuse the machinery with _helperlib to expose C functions. The manual registration of str2int here will not work with ahead-of-time compiled code.

[stuartarchibald]

Thanks for the PR. Further to @sklam's comments, having taken a quick look at this, I'm of the view that testing that is considerably more extensive needs writing ahead of re-review. Thanks.

[shssf]

@sklam is this approach did you mean?
@stuartarchibald I did changes I mentioned. The code is not working. Please help.

[stuartarchibald]

@shssf Yes, the approach presented is far closer to that needed for use in Numba, thanks.

Following on from gitter chat, I've added this branch https://github.com/stuartarchibald/numba/tree/pr_3937 which has stuartarchibald@a9bf601 as a patch to fix and demo the code working:

./runtests.py numba.tests.test_unicode.TestUnicodeStr2Int.test_str2int_demo

there are still a large number of things that are working essentially by accident, but I figure it's probably easier to fix something that half works with a bunch of problems than something that doesn't. I also added a load of comments to the code to try and explain literally what is going on, this however doesn't cover commenting on the issues in the current code. Main issues at present are 1) assumption that strings are equivalent to char *, which they are not, Numba's internal representation handles unicode! :) 2) Most of the types need more thought to make it work everywhere. 3) Overloading int will need more thought in general.

Hope this helps, we can definitely discuss more.

[shssf]

@stuartarchibald Thank you very much!
Are we going to support kind-2 and kind-4 in string conversion procedure? If so, could you recommend where I can find good examples of digits represented in unicode kind-4 for tests?

[stuartarchibald]

@shssf No problem. In terms of kind-2 and kind-4 strings, I think that yes this needs to work, consider the following:

from numba import njit

@njit
def foo():
    string = "🐍⚡123"
    part = string[2:]
    return int(part)

print(foo.py_func())
print(foo())

the above is something that could reasonably occur in practice but will be incorrectly handled by the current code. Further, I'm not convinced that the strto*(3) family produces sufficiently equivalent behaviour to that of the CPython implementation.

[shssf]

@stuartarchibald In this case part = string[2:] Numba will not support other digits like this https://ltl-taiwan.com/chinese-numbers/#chapter-1

[stuartarchibald]

@shssf The purpose of the demonstration was to highlight that wider kind unicode representations of numbers that could be encoded as ASCII are possible and trivial to achieve.

@njit
def foo():
    string = "🐍⚡123" # Unicode kind-2
    part = string[2:] # This slice is also unicode kind-2
    return int(part) # the `int` call now has to handle a unicode kind-2 repr of "123"

Numba will not support other digits like this https://ltl-taiwan.com/chinese-numbers/#chapter-1

Correct, Numba has to reproduce exactly what CPython does, if CPython doesn't support it then Numba doesn't have to either.

[stuartarchibald]

Just checking in on the state of this PR. @shssf @Alexander-Makaryev was wondering if you have time to address the changes above, if not, don't worry, one of the core developers can take over to do the fixes. Thanks.

[shssf]

@stuartarchibald Sorry we ran out of time to continue this.

[stuartarchibald]

@shssf thanks, am going to close this for now, there's merge conflicts and additional effort required to fully support unicode strings. Please feel free to reopen if updates are made to address these concerns. Thanks again.