Skip to content

Conversation

laurensvalk
Copy link
Contributor

@laurensvalk laurensvalk commented Aug 6, 2021

Allows parsing of a complex string with a similar result as CPython.

Until now, only strings like 3j were supported, with strings like 1+2j giving a syntax error.

The following strings will now also work.

print(complex("1+2j"))
print(complex("+1+2j"))
print(complex("-1+2j"))

Cases with only a sign at [0] or no sign at all are treated as they already were.

Checks are added to force A to be real and B to be imaginary. This raises exceptions for the same cases as CPython, like Aj+B, even though MicroPython would otherwise return the correct result.

Fixes this MicroPython forum post.

Edit: Fix commit message < 75 characters per line.

Signed-off-by: Laurens Valk laurens@pybricks.com

This works by splitting the string at the non-leading sign.
The following strings will work.

print(complex("1+2j"))
print(complex("+1+2j"))
print(complex("-1+2j"))

Cases without a non-leading sign will be treated as they already were.

Checks are added to force A to be real and B to be imaginary. This
raises exceptions for the same cases as CPython, like Aj+B, even though
MicroPython would otherwise return the correct result.

Signed-off-by: Laurens Valk <laurens@pybricks.com>
@codecov-commenter
Copy link

codecov-commenter commented Aug 6, 2021

Codecov Report

Merging #7623 (84bb633) into master (a367529) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7623      +/-   ##
==========================================
- Coverage   98.27%   98.27%   -0.01%     
==========================================
  Files         154      154              
  Lines       19994    20001       +7     
==========================================
+ Hits        19650    19656       +6     
- Misses        344      345       +1     
Impacted Files Coverage Δ
py/objcomplex.c 100.00% <100.00%> (ø)
py/runtime.c 99.24% <0.00%> (-0.16%) ⬇️
py/obj.c 96.03% <0.00%> (ø)
py/objenumerate.c 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a367529...84bb633. Read the comment docs.

@jimmo
Copy link
Member

jimmo commented Aug 6, 2021

Thanks!

This is +96 bytes on PYBV11. I notice that objcomplex.c is the only place that sets the force_complex=True when calling mp_parse_num_decimal so I wondered whether this could be implemented more efficiently in mp_parse_num_decimal instead (when force_complex=True is set).

Here's an attempt at doing this: ae8a38e Comes in at +64 bytes.

This also deals with an extra case where there's leading spaces (i.e. the +/- for the real component may not be the first character, e.g. complex(" -1-2j")). Although it does mean we now accept complex("1 +2j") which CPython doesn't.

@dpgeorge dpgeorge added the py-core Relates to py/ directory in source label Aug 10, 2021
@dpgeorge
Copy link
Member

@jimmo 's version does look slightly more efficient, and it only creates one complex object whereas the version here creates 2 and then adds them (creating a third on the heap).

What do you think @laurensvalk ?

@laurensvalk
Copy link
Contributor Author

Thanks for your quick responses. I'm all for @jimmo's more efficient implementation, so feel free to close this one.

For context, I came across this while documenting the MicroPython modules in more detail (still a work in progress) for use with Pybricks. Our plan is to provide typing and autocomplete in our online editor.

image

@dpgeorge
Copy link
Member

I took @jimmo 's attempt above and extended it, and made it #8799. I'll close this PR in favour of that one (although it's still uncertain if it'll be merged or not).

@dpgeorge dpgeorge closed this Jun 21, 2022
RetiredWizard pushed a commit to RetiredWizard/micropython that referenced this pull request Feb 22, 2023
Correctly raise OS error in socketpool_socket_recv_into()
tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Mar 24, 2023
This reverts commit 7e6e824.

Fixes micropython#7770

The change in micropython#7623 needs to be revered; the raise-site added in micropython#7632
is the correct one and the one in socketpool needs to be reverted.

This is not affecting 8.0.x because micropython#7623 was not back-ported to there
before we realized it was not a full fix.

Both micropython#7770 and micropython#7606 should be re-tested. I didn't test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
py-core Relates to py/ directory in source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants