Skip to content
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

Rational calc value error #24543

Closed
ChisenZhang opened this issue Jan 18, 2023 · 3 comments · Fixed by #24562
Closed

Rational calc value error #24543

ChisenZhang opened this issue Jan 18, 2023 · 3 comments · Fixed by #24562
Labels
core.numbers Easy to Fix This is a good issue for new contributors. Feel free to work on this if no one else has already.

Comments

@ChisenZhang
Copy link

python 3.11, sympy 1.11.1
when calc Rational('0.5', '100'), the value is 1/100100; but Rational(0.5, 100) the value is 1/200, this value is the true value, and the version of sympy 1.8 is normal

@ChisenZhang ChisenZhang changed the title Ratinal calc value error Rational calc value error Jan 18, 2023
@oscarbenjamin
Copy link
Contributor

This should probably raise an error. The expected way to do this is:

In [1]: Rational('0.5/100')
Out[1]: 1/200

In [2]: Rational('0.5') / Rational('100')
Out[2]: 1/200

skirpichev added a commit to diofant/diofant that referenced this issue Jan 19, 2023
@sylee957
Copy link
Member

sylee957 commented Jan 20, 2023

Actually, this should be a flaw in the logic because string is multiplied somewhere in

sympy/sympy/core/numbers.py

Lines 1628 to 1640 in 68803e5

if not isinstance(p, SYMPY_INTS):
p = Rational(p)
q *= p.q
p = p.p
else:
p = int(p)
if not isinstance(q, SYMPY_INTS):
q = Rational(q)
p *= q.q
q = q.p
else:
q = int(q)

@smichr
Copy link
Member

smichr commented Jan 20, 2023

The logic should like like this:

        Q = 1
        gcd = 1

        if not isinstance(p, SYMPY_INTS):
            p = Rational(p)
            Q *= p.q
            p = p.p
        else:
            p = int(p)

        if not isinstance(q, SYMPY_INTS):
            q = Rational(q)
            p *= q.q
            Q *= q.p
        else:
            Q *= int(q)
        q = Q

A test can be added:

for p in ('1.5', 1.5, 2):
    for q in ('1.5', 1.5, 2):
        assert Rational(p, q).as_numer_denom() == Rational('%s/%s'%(p,q)).as_numer_denom()

@smichr smichr added the Easy to Fix This is a good issue for new contributors. Feel free to work on this if no one else has already. label Jan 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core.numbers Easy to Fix This is a good issue for new contributors. Feel free to work on this if no one else has already.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants