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

runtime error with simple expression #29156

Open
zimmermann6 opened this issue Feb 5, 2020 · 21 comments
Open

runtime error with simple expression #29156

zimmermann6 opened this issue Feb 5, 2020 · 21 comments

Comments

@zimmermann6
Copy link

sage: 2^(-21111111111/11111111111)
...
RuntimeError: 

Why is Sage unable to evaluate this expression?

Depends on #30446
Depends on #31118

Upstream: Fixed upstream, in a later stable release.

Component: basic arithmetic

Author: Ben Livingston, Dave Morris

Branch/Commit: public/29156 @ 09e62e3

Reviewer: Kwankyu Lee

Issue created by migration from https://trac.sagemath.org/ticket/29156

@mkoeppe
Copy link
Member

mkoeppe commented May 1, 2020

comment:1

Moving tickets to milestone sage-9.2 based on a review of last modification date, branch status, and severity.

@mkoeppe mkoeppe modified the milestones: sage-9.1, sage-9.2 May 1, 2020
@bmlivin bmlivin mannequin self-assigned this Sep 14, 2020
@bmlivin
Copy link
Mannequin

bmlivin mannequin commented Sep 20, 2020

comment:4

This is something that might be a bug in pynac, or it might be that Sage is using pynac incorrectly. Here is an excerpt from pynac's numeric::power in numeric.cpp, which is what ends up being called in this example:

if (exponent.is_negative()) {
    long int_exp = -(expo.to_long());
    ...
}

In your example, exponent is what you think it should be (-21111111111/11111111111). Here's what to_long does:

if (mpz_fits_sint_p(v._bigint)) {
    long n = mpz_get_si(bigint);
    mpz_clear(bigint);
    return n;
}
mpz_clear(bigint);
throw conversion_error();

Here, v._bigint is the absolute value of the exponent's numerator, 21111111111. That happens to be a bit larger than 32 bits, so the if statement doesn't execute, and throw_conversion_error is called.

I haven't combed through pynac's documentation to see whether they point out that numeric::power shouldn't be called with negative rational exponents whose numerators are greater than 32 bits, but I can't see why they would want to do this and not do the same with positive numerators or denominators greater than 32 bits. So I think this is a bug in pynac.

@bmlivin
Copy link
Mannequin

bmlivin mannequin commented Sep 20, 2020

comment:5

It may be a while before I can report this upstream. If someone else would like to do so, please go ahead.

@bmlivin
Copy link
Mannequin

bmlivin mannequin commented Sep 20, 2020

Upstream: Not yet reported upstream; Will do shortly.

@bmlivin
Copy link
Mannequin

bmlivin mannequin commented Oct 12, 2020

comment:6

See pynac/pynac#356 for the upstream report.

@bmlivin
Copy link
Mannequin

bmlivin mannequin commented Oct 12, 2020

Changed upstream from Not yet reported upstream; Will do shortly. to Reported upstream. No feedback yet.

@mkoeppe mkoeppe modified the milestones: sage-9.2, sage-9.3 Oct 24, 2020
@bmlivin
Copy link
Mannequin

bmlivin mannequin commented Nov 1, 2020

Changed upstream from Reported upstream. No feedback yet. to Fixed upstream, in a later stable release.

@bmlivin
Copy link
Mannequin

bmlivin mannequin commented Nov 1, 2020

comment:8

I've submitted a pull request for this with pynac: pynac/pynac#358. That said, my pull request does not fix the underlying issue of dealing with negative exponents. It only loosens the requirement from the numerator needing to fit in a C int to needing to fit in a C long. That fixes the specific example reported in this ticket, but if someone wanted to raise to a rational power with a numerator greater than 63 bits, it would still raise an error.

@bmlivin
Copy link
Mannequin

bmlivin mannequin commented Jan 31, 2021

comment:9

This should be fixed by the new commits to #30446. Since they're not merged in yet, I'm not sure whether to change this to needs review or not.

@bmlivin
Copy link
Mannequin

bmlivin mannequin commented Jan 31, 2021

Dependencies: #30446, #31118

@bmlivin
Copy link
Mannequin

bmlivin mannequin commented Jan 31, 2021

Author: Ben Livingston

@zimmermann6
Copy link
Author

comment:12

thanks let's look again once #30446 is merged

@DaveWitteMorris
Copy link
Member

Branch: public/29156

@DaveWitteMorris
Copy link
Member

Changed author from Ben Livingston to Ben Livingston, Dave Morris

@DaveWitteMorris
Copy link
Member

Commit: 09e62e3

@DaveWitteMorris
Copy link
Member

comment:14

This no longer gives an error, because it was fixed by the pynac update in #30446, as mentioned above. The PR just adds a doctest.


New commits:

09e62e3trac 29156 doctest

@kwankyu
Copy link
Collaborator

kwankyu commented Mar 12, 2021

Reviewer: Kwankyu Lee

@kwankyu
Copy link
Collaborator

kwankyu commented Mar 12, 2021

comment:16

Looks good.

@DaveWitteMorris
Copy link
Member

comment:17

Thanks!

@vbraun
Copy link
Member

vbraun commented Mar 17, 2021

comment:18

On 32-bit:

**********************************************************************
File "src/sage/rings/integer.pyx", line 2201, in sage.rings.integer.Integer.__pow__
Failed example:
    2^(-21111111111/11111111111)
Exception raised:
    Traceback (most recent call last):
      File "/home/buildbot/slave/sage_git/build/local/lib/python3.9/site-packages/sage/doctest/forker.py", line 714, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/buildbot/slave/sage_git/build/local/lib/python3.9/site-packages/sage/doctest/forker.py", line 1133, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.rings.integer.Integer.__pow__[22]>", line 1, in <module>
        Integer(2)**(-Integer(21111111111)/Integer(11111111111))
      File "sage/rings/integer.pyx", line 2211, in sage.rings.integer.Integer.__pow__ (build/cythonized/sage/rings/integer.c:15193)
        return coercion_model.bin_op(left, right, operator.pow)
      File "sage/structure/coerce.pyx", line 1204, in sage.structure.coerce.CoercionModel.bin_op (build/cythonized/sage/structure/coerce.c:10671)
        return PyObject_CallObject(op, xy)
      File "sage/structure/element.pyx", line 2055, in sage.structure.element.Element.__pow__ (build/cythonized/sage/structure/element.c:14428)
        return (<Element>left)._pow_(right)
      File "sage/rings/rational.pyx", line 2602, in sage.rings.rational.Rational._pow_ (build/cythonized/sage/rings/rational.cpp:22251)
        return SR(c) * SR(d).power(n, hold=True)
      File "sage/structure/element.pyx", line 1513, in sage.structure.element.Element.__mul__ (build/cythonized/sage/structure/element.c:12170)
        return (<Element>left)._mul_(right)
      File "sage/symbolic/expression.pyx", line 3799, in sage.symbolic.expression.Expression._mul_ (build/cythonized/sage/symbolic/expression.cpp:25489)
        x = left._gobj * _right._gobj
    RuntimeError
**********************************************************************

@mkoeppe
Copy link
Member

mkoeppe commented May 10, 2021

comment:19

Moving to 9.4, as 9.3 has been released.

@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.4 May 10, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.4, sage-9.5 Aug 22, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Dec 18, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.6, sage-9.7 May 3, 2022
@mkoeppe mkoeppe modified the milestones: sage-9.7, sage-9.8 Sep 19, 2022
@mkoeppe mkoeppe removed this from the sage-9.8 milestone Jan 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants