Added support for round on integers. #3557
Conversation
|
I will probably need some help to understand why did tests failed. I had error in first commit but I corected it in second one. The only think I see failing in test now is increse in size (I think that I can't avoid it) and some problems with downloads. |
|
There is a mistake in my code, please wait with request until I coreect it. |
|
I corrected my previous mistake to make round work same way as its Python3 version (toward even choice in case of same distances). I also updated tests so this mistake would be detected. One of the check is still failing but I dont understand why. Can anyone give me any clues? (This is my first pull request for micropython). Tests run by make test are all clear. |
|
It's the code size check: |
|
Thanks for the contribution. As @peterhinch points out, the error is because this addition increases the size of the minimal port. It would be interesting to see if you can modify the code to reduce its size. I'll give some hints for this in the PR itself. |
py/modbuiltins.c
Outdated
| } | ||
| if (!MP_OBJ_IS_INT(args[1])){ | ||
| mp_raise_TypeError("Second parameter must be an integer"); | ||
| } |
There was a problem hiding this comment.
Instead of doing this check for int, just extract the int like mp_int_t num_dig = mp_obj_get_int(args[1]);. That will raise an exception if the arg is not an int. Then you'll also have the int value as a machine int which will make the rest of the code simpler (it won't allow bignum for this arg but it's unlikely that one will ever pass huge values to the number of digits).
py/modbuiltins.c
Outdated
| @@ -460,8 +460,33 @@ MP_DEFINE_CONST_FUN_OBJ_1(mp_builtin_repr_obj, mp_builtin_repr); | |||
| STATIC mp_obj_t mp_builtin_round(size_t n_args, const mp_obj_t *args) { | |||
| mp_obj_t o_in = args[0]; | |||
| if (MP_OBJ_IS_INT(o_in)) { | |||
| return o_in; | |||
| } | |||
| if(n_args == 1 || mp_obj_is_true(mp_binary_op(MP_BINARY_OP_MORE_EQUAL, args[1], mp_obj_new_int(0)))){ | |||
There was a problem hiding this comment.
If you have the value ofnum_dig (see below) then this second check can be much simpler, like num_dig >= 0.
py/modbuiltins.c
Outdated
| mp_raise_TypeError("Second parameter must be an integer"); | ||
| } | ||
| mp_obj_t num_dig = mp_unary_op(MP_UNARY_OP_NEGATIVE, (args[1])); | ||
| mp_obj_t mult = mp_binary_op(MP_BINARY_OP_POWER, mp_obj_new_int(10), num_dig); |
There was a problem hiding this comment.
You can use MP_OBJ_NEW_SMALL_INT(10) to make it simpler and more efficient. Then also MP_OBJ_NEW_SMALL_INT(-num_dig).
py/modbuiltins.c
Outdated
| } | ||
| mp_obj_t num_dig = mp_unary_op(MP_UNARY_OP_NEGATIVE, (args[1])); | ||
| mp_obj_t mult = mp_binary_op(MP_BINARY_OP_POWER, mp_obj_new_int(10), num_dig); | ||
| mp_obj_t half_mult = mp_binary_op(MP_BINARY_OP_FLOOR_DIVIDE, mult, mp_obj_new_int(2)); |
py/modbuiltins.c
Outdated
| mp_obj_t floor = mp_binary_op(MP_BINARY_OP_FLOOR_DIVIDE, o_in, mult); | ||
| mp_obj_t modulo = mp_binary_op(MP_BINARY_OP_MODULO, o_in, mult); | ||
| if(mp_obj_is_true(mp_binary_op(MP_B 8000 INARY_OP_MORE, modulo, half_mult))){ | ||
| return mp_binary_op(MP_BINARY_OP_ADD, mp_binary_op(MP_BINARY_OP_MULTIPLY, floor, mult), mult); |
There was a problem hiding this comment.
It seems like you always need to do mp_binary_op(MP_BINARY_OP_MULTIPLY, floor, mult) so that can be factored out of this if logic.
|
|
||
| tests = [ | ||
| False, True, | ||
| 0, 1, -1, 10 | ||
| (False,), (True,), |
There was a problem hiding this comment.
Did you test passing a bool as well as a second arg?
|
Thank you for your feedback. I will address your suggestions and I will send new commit. |
|
@honza-klu I see you pushed some changes. Is this now ready to review again? |
|
I addressed all given hints. Code is smaller (but it still trigger increased size error in the test). I think it is ready for a review. |
|
Thanks @honza-klu for updating the PR. I've squashed and merged these changes in b318ebf with some edits:
|
This patch add support for rounding integers same way as CPython.
This patch solve issue #3538.