Skip to content

Fix complex parsing signs #10297

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

ComplexSymbol
Copy link

Changes

I tried to fix a parsing error in the py/parsenum.c file.

Tests:
Before: print(complex('-9e-17+1j')) # prints (-9e-17-1j) wrong
After: print(complex('-9e-17+1j')) # prints (-9e-17+1j) correct

It's pretty simple, I just made it so it resets dec_neg upon restarting the parse for complex numbers so it doesn't carry through to the imaginary part. dec_neg gets applied to the real part before moving on to the imaginary, so resetting it doesn't change the sign of the real part.

Other stuff

Note: The contributions listed here on my fork are at the main branch, the other (doubles-cmath) branch on my fork is for my project, not contributing.

This is my first time contributing to anything! I'm not sure whether to submit this to main or the latest branch.

Before:  print(complex('-9e-17+1j'))  # prints (-9e-17-1j)
After:  print(complex('-9e-17+1j')) # prints (-9e-17+1j)
@dhalbert
Copy link
Collaborator

Thanks for this! Submitting on main is fine: we are not planning further 9.2.x releases except for egregious errors.

This code comes from MicroPython, so it would be appropriate to submit this upstream, to https://github.com/micropython/micropython. It's worth looking to see if the code there has already been fixed or is different in some way.

@Neradoc
Copy link

Neradoc commented Apr 26, 2025

Yeah I was looking at MP and I see the same in the code, and same results when trying on 1.25.

>>> complex("1+j")
(1+1j)
>>> complex("-1+j")
(-1-1j)

@ComplexSymbol
Copy link
Author

Thanks for this! Submitting on main is fine: we are not planning further 9.2.x releases except for egregious errors.

This code comes from MicroPython, so it would be appropriate to submit this upstream, to https://github.com/micropython/micropython. It's worth looking to see if the code there has already been fixed or is different in some way.

Should I submit a PR over there too then?

@dhalbert
Copy link
Collaborator

Should I submit a PR over there too then?

Yes, submit there first. If they accept it (and they might change it a bit), then we'll take their change as it is merged there, so the sources match. If they don't do it promptly, we can merge it here and then catch up with what they do.

@ComplexSymbol
Copy link
Author

Should I submit a PR over there too then?

Yes, submit there first. If they accept it (and they might change it a bit), then we'll take their change as it is merged there, so the sources match. If they don't do it promptly, we can merge it here and then catch up with what they do.

Ok, thanks!

@dhalbert
Copy link
Collaborator

Another thing to do is to add a test: add it to tests/float/complex1.py. You can just add a few more cases to the set of print statements at the beginning. The standard test automation runs the test in micropython and in regular CPython, and the results should be the same. (It's also possible to specify the expected results with a .exp file, but I think the first case should be fine here for simple floats with e notation that can be represented exactly.)

@ComplexSymbol
Copy link
Author

Another thing to do is to add a test: add it to tests/float/complex1.py. You can just add a few more cases to the set of print statements at the beginning. The standard test automation runs the test in micropython and in regular CPython, and the results should be the same. (It's also possible to specify the expected results with a .exp file, but I think the first case should be fine here for simple floats with e notation that can be represented exactly.)

There are already tests there that should've caught this mistake, not sure why they weren't noticed... There are missing tests for exponential notation and complex numbers, should I add those to to the complex1.py tests or somewhere else?

@dhalbert
Copy link
Collaborator

There is no test like complex("-1+1j") (negative followed by positive) in complex1.py. That seems to me to be just an oversight.

It would be fine to add more tests, and that's the right file to add them in. Won't hurt.

@ComplexSymbol
Copy link
Author

There is no test like complex("-1+1j") (negative followed by positive) in complex1.py. That seems to me to be just an oversight.

It would be fine to add more tests, and that's the right file to add them in. Won't hurt.

Whoops, I thought I saw something like that, anyways I'll go ahead and add those. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants