Opened 13 months ago

Closed 6 months ago

Last modified 6 months ago

#30446 closed defect (fixed)

Update pynac to 0.7.27 to fix wrong result on symbolic exponentiation

Reported by: dkrenn Owned by:
Priority: critical Milestone: sage-9.3
Component: symbolics Keywords:
Cc: zimmerma Merged in:
Authors: Ben Livingston, Dima Pasechnik Reviewers: Dima Pasechnik, Matthias Koeppe
Report Upstream: None of the above - read trac for reasoning. Work issues:
Branch: ccdf77c (Commits, GitHub, GitLab) Commit:
Dependencies: #31118 Stopgaps:

Status badges

Description (last modified by slelievre)

Before this ticket:

sage: n((24*sqrt(3))^(100/50))
3.80392047155301e5927962146
sage: n((24*sqrt(3))^(2))
1728.00000000000

Clearly, both should be the same, namely the second.

This was on SageMath 9.1 on openSUSE Leap 15.2 (64bit) (reported by a colleague to me).

I've tried to reproduce, but my SageMath, as well as on CoCalc crashes. The error message on my machine was

Traceback (most recent call last):
  File "<string>", line 25, in <module>
ModuleNotFoundError: No module named 'Cython'
Error while executing Python code.
Saved trace to /home/dakrenn/.sage/crash_logs/crash_lqkqgydq.log
------------------------------------------------------------------------
Unhandled SIGABRT: An abort() occurred.
This probably occurred because a *compiled* module has a bug
in it and is not properly wrapped with sig_on(), sig_off().
Python will now terminate.
------------------------------------------------------------------------
Aborted (core dumped)

However, my SageMath seems to be fine otherwise, make ptestlong passes. Same on current 9.2.beta9.

Change History (41)

comment:1 Changed 13 months ago by gh-DaveWitteMorris

I confirm the error. I get exactly the same answers as in the original report. I am using Mac OS 10.15.5, and tested both 9.1 and 9.2b10. (The version with 100/2 is slow -- takes about 50 seconds.)

When I tried CoCalc, it failed because it ran out of memory. (The memory gauge was over 3GB before it aborted.)

comment:2 Changed 12 months ago by gh-bmlivin

  • Owner changed from (none) to gh-bmlivin

comment:3 Changed 12 months ago by gh-DaveWitteMorris

There is no need to use big numbers, so the situation is even worse than it first appeared:

sage: n((2*sqrt(2))^(2/1))                                                      
1.24131221754531e1292913987
sage: n((2*sqrt(2))^2)                                                          
8.00000000000000

This only takes a few seconds with 9.1 on CoCalc (but needs more than 1GB of memory) or with 9.2b13 on MacOS 10.15.6.

comment:4 Changed 12 months ago by mantepse

I can confirm this on Ubuntu 18 with SageMath version 9.2.beta13, using Python 3.8.5.

comment:5 Changed 12 months ago by gh-bmlivin

  • Owner changed from gh-bmlivin to (none)
  • Report Upstream changed from N/A to Not yet reported upstream; Will do shortly.

Okay, I believe I've tracked this down. It is a pynac bug. Here's what's happening:

Sage eventually calls pynac's pow from sage.symbolic.expression:Expression._pow_, but before that happens, it simplifies the exponent. Specifically, it simplifies the exponent so that pynac thinks it is an integer, but its type is still MPQ.

None of that should matter, except that what pynac calls to evaluate the expression is numeric::pow_intexp. Here is the content of that function:

if (not exponent.is_integer())
    throw std::runtime_error("nueric::pow_intexp: exponent not integer");
if (exponent.t == MPZ) {
    if (not mpz_fits_sint_p(exponent.v._bigint))
        throw std::runtime_error("size of exponent exceeds signed long size");
    return power(mpz_get_si(exponent.v._bigint));
}
return power(exponent.v._long);

In this function, exponent.is_integer() is True and exponent.t is MPQ. So both of the if statements are false, and power(exponent.v._long) is called. The problem is that v._long contains junk (very large junk!). Evaluating that statement takes a long time, and it won't give you the answer you want.

If, on the other hand, the second if statement were true, the result would have been exactly what you want. So simply checking whether exponent.t == MPQ or exponent.t == MPZ should fix this. I haven't tried this, but it's what I would at least try.

I think it may be possible as well to fix this by getting Sage to change the type of the exponent when it's cancelled out. I haven't looked into this and I think it sounds like a bad idea.

I'm unassigning this because I can't currently report it upstream.

comment:6 Changed 11 months ago by gh-bmlivin

  • Authors set to Ben Livingston
  • Report Upstream changed from Not yet reported upstream; Will do shortly. to Fixed upstream, in a later stable release.
  • Status changed from new to needs_review

I've fixed this upstream and submitted a pull request: https://github.com/pynac/pynac/pull/355. The pull request also includes additions to the doctest for sage.symbolic.expression.Expression::_pow_.

comment:7 Changed 11 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:8 Changed 8 months ago by dimpase

OK, trying https://patch-diff.githubusercontent.com/raw/pynac/pynac/pull/355.patch now.

But where are "additions to the doctest for sage.symbolic.expression.Expression::_pow_"?

I presume they should be here, on this ticket.

comment:9 Changed 8 months ago by dimpase

OK, with that patch,

sage: QQ((24*sqrt(3))^(100/50))==1728                                                                                                                                
True

works. I'll add this as a doctest somewhere.

comment:10 Changed 8 months ago by dimpase

  • Branch set to u/dimpase/packages/pynac/30446
  • Commit set to dc073dfa9224efb61dc01d49ef60a9c7fa0dc568
  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

patches from pynac PRs to be removed at the next pynac update.


New commits:

dc073dffixes for trac #30446

comment:11 Changed 8 months ago by gh-DaveWitteMorris

This patch to pynac also fixes #28620, #30304, and #30786. The PR at #30786 adds doctests for all 3 of these tickets (and also moves the doctest of this ticket from the _latex_ method to the _pow_ method, which seems like a more appropriate place).

comment:12 Changed 8 months ago by gh-DaveWitteMorris

fyi: Patchbot Linux/74-Ubuntu_SMP_Tue_Sep_17_17%3A06%3A04_UTC_2019/x86_64/4.15.0-65-generic/pc72 seems to have failed to build pynac at ticket #30786. ("Error installing package pynac-0.7.26.sage-2020-04-03.p1") If there is a genuine problem, I think it must be because of this ticket.

comment:13 Changed 8 months ago by gh-DaveWitteMorris

This patch also seems to fix #31137. I added a doctest there.

comment:14 Changed 8 months ago by gh-DaveWitteMorris

fyi: Another ubuntu patchbot failed to build pynac (on ticket #31137 this time). Both failures say WARNING: 'aclocal-1.16' is missing on your system. ... Makefile:432: recipe for target 'aclocal.m4' failed.

comment:15 Changed 8 months ago by vbraun

  • Status changed from positive_review to needs_work

Patch touches configure stuff which triggers automake run (which is not a Sage dependency)

Building pynac-0.7.26.sage-2020-04-03.p1
CDPATH="${ZSH_VERSION+.}:" && cd . && /bin/bash /var/lib/buildbot/slave/sage_git/build/local/var/tmp/sage/build/pynac-0.7.26.sage-2020-04-03.p1/src/missing aclocal-1.16 -I m4
/var/lib/buildbot/slave/sage_git/build/local/var/tmp/sage/build/pynac-0.7.26.sage-2020-04-03.p1/src/missing: line 81: aclocal-1.16: command not found
WARNING: 'aclocal-1.16' is missing on your system.
         You should only need it if you modified 'acinclude.m4' or
         'configure.ac' or m4 files included by 'configure.ac'.
         The 'aclocal' program is part of the GNU Automake package:
         <https://www.gnu.org/software/automake>
         It also requires GNU Autoconf, GNU m4 and Perl in order to run:
         <https://www.gnu.org/software/autoconf>
         <https://www.gnu.org/software/m4/>
         <https://www.perl.org/>
Makefile:432: recipe for target 'aclocal.m4' failed
make[5]: *** [aclocal.m4] Error 127
make[5]: Failed to remake makefile 'Makefile'.
make[5]: Target 'all' not remade because of errors.

comment:16 Changed 8 months ago by gh-mjungmath

  • Priority changed from major to critical

I set this to critical since many tickets (some already positively reviewed providing a doctest) rely on this one.

comment:17 Changed 8 months ago by mkoeppe

https://doc.sagemath.org/html/en/developer/packaging.html#when-to-patch-when-to-repackage-when-to-autoconfiscate explains that patching is not an option here. A pynac release needs to be made and the new release tarball needs to be used.

comment:18 Changed 8 months ago by dimpase

  • Dependencies set to #31118

I'll repurpose this ticket for the package update.

comment:19 Changed 8 months ago by dimpase

  • Branch changed from u/dimpase/packages/pynac/30446 to u/dimpase/packages/pynac/0727
  • Commit changed from dc073dfa9224efb61dc01d49ef60a9c7fa0dc568 to 563940e8554fe96a16fdf669c03f895aacf27b3c
  • Report Upstream changed from Fixed upstream, in a later stable release. to None of the above - read trac for reasoning.
  • Status changed from needs_work to needs_review

New commits:

9d747fcsage --package update-latest: Distinguish pypi package name and spkg name
f74f66csage --package update-latest: Accept package classes :standard:, :optional: etc., restrict to normal Python packages
182b3d2sage -package fix-checksum: Handle package classes, ignore non-normal packages
9a57cf6pynac update
563940edoctest from #30446

comment:20 follow-up: Changed 8 months ago by mkoeppe

When you make official releases in https://github.com/pynac/pynac, there is no need for a version suffix such as "sage-2020-04-03". I used this when I made the previous release from https://github.com/mkoeppe/pynac to make clear that this was an unofficial release.

comment:21 Changed 8 months ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:22 Changed 8 months ago by git

  • Commit changed from 563940e8554fe96a16fdf669c03f895aacf27b3c to 51def3cc9203861a11404019d68ed90937d9f017

Branch pushed to git repo; I updated commit sha1. New commits:

51def3csane version name

comment:23 Changed 8 months ago by git

  • Commit changed from 51def3cc9203861a11404019d68ed90937d9f017 to 4834dc8a7ab300c57921986e847972030b0547ca

Branch pushed to git repo; I updated commit sha1. New commits:

4834dc8remove upstreamed patch

comment:24 Changed 8 months ago by dimpase

  • Status changed from needs_work to needs_review

comment:25 Changed 8 months ago by dimpase

  • Status changed from needs_review to needs_work

should provide a cleaner tarball

comment:26 in reply to: ↑ 20 Changed 8 months ago by dimpase

Replying to mkoeppe:

When you make official releases in https://github.com/pynac/pynac, there is no need for a version suffix such as "sage-2020-04-03". I used this when I made the previous release from https://github.com/mkoeppe/pynac to make clear that this was an unofficial release.

but then https://github.com/pynac/pynac/commit/bbaff9d21d31e73f74bf3dd751b16c4dd3ac28a7 has sneaked upstream in some way :-)

comment:27 Changed 8 months ago by git

  • Commit changed from 4834dc8a7ab300c57921986e847972030b0547ca to 214712de4fd44f9e703cfffc7cccced7b5757a12

Branch pushed to git repo; I updated commit sha1. New commits:

214712dtarball update

comment:28 Changed 8 months ago by dimpase

  • Authors changed from Ben Livingston to Ben Livingston, Dima Pasechnik
  • Status changed from needs_work to needs_review

comment:29 Changed 8 months ago by mkoeppe

build/pkgs/pynac/SPKG.rst could use an update. In particular "Special Update/Build? Instructions" should be removed

comment:30 Changed 8 months ago by mkoeppe

  • Reviewers changed from Dima Pasechnik to Dima Pasechnik, Matthias Koeppe

comment:31 Changed 8 months ago by mkoeppe

  • Status changed from needs_review to positive_review

comment:32 Changed 8 months ago by mkoeppe

  • Summary changed from wrong result on symbolic exponentiation to Update pynac to 0.7.27 to fix wrong result on symbolic exponentiation

comment:33 Changed 8 months ago by git

  • Commit changed from 214712de4fd44f9e703cfffc7cccced7b5757a12 to ccdf77cba4f57f2e8f595dbeefb2fb604d9f9ace
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

ccdf77cupdate SPKG.rst

comment:34 Changed 8 months ago by dimpase

  • Status changed from needs_review to positive_review

ok, done

comment:35 Changed 8 months ago by zimmerma

  • Cc zimmerma added

comment:36 Changed 7 months ago by vbraun

  • Status changed from positive_review to needs_work
[dochtml] [calculus ] docstring of sage.symbolic.expression.Expression.subs:135: WARNING: Literal block expected; none found.
[dochtml] [calculus ] docstring of sage.symbolic.expression.Expression.substitute:135: WARNING: Literal block expected; none found.
[dochtml] [manifolds] The inventory files are in local/share/doc/sage/inventory/en/reference/manifolds.
[dochtml] Build finished. The built documents can be found in /home/release/Sage/local/share/doc/sage/inventory/en/reference/manifolds
[dochtml] [calculus ] The inventory files are in local/share/doc/sage/inventory/en/reference/calculus.
[dochtml] Error building the documentation.
[dochtml] Traceback (most recent call last):
[dochtml]   File "/usr/lib64/python3.9/runpy.py", line 197, in _run_module_as_main
[dochtml]     return _run_code(code, main_globals, None,
[dochtml]   File "/usr/lib64/python3.9/runpy.py", line 87, in _run_code
[dochtml]     exec(code, run_globals)
[dochtml]   File "/home/release/Sage/local/lib64/python3.9/site-packages/sage_setup/docbuild/__main__.py", line 2, in <module>
[dochtml]     main()
[dochtml]   File "/home/release/Sage/local/lib64/python3.9/site-packages/sage_setup/docbuild/__init__.py", line 1730, in main
[dochtml]     builder()
[dochtml]   File "/home/release/Sage/local/lib64/python3.9/site-packages/sage_setup/docbuild/__init__.py", line 343, in _wrapper
[dochtml]     getattr(get_builder(document), 'inventory')(*args, **kwds)
[dochtml]   File "/home/release/Sage/local/lib64/python3.9/site-packages/sage_setup/docbuild/__init__.py", line 569, in _wrapper
[dochtml]     self._build_everything_except_bibliography(lang, format, *args, **kwds)
[dochtml]   File "/home/release/Sage/local/lib64/python3.9/site-packages/sage_setup/docbuild/__init__.py", line 555, in _build_everything_except_bibliography
[dochtml]     build_many(build_ref_doc, non_references)
[dochtml]   File "/home/release/Sage/local/lib64/python3.9/site-packages/sage_setup/docbuild/__init__.py", line 295, in build_many
[dochtml]     _build_many(target, args, processes=NUM_THREADS)
[dochtml]   File "/home/release/Sage/local/lib64/python3.9/site-packages/sage_setup/docbuild/utils.py", line 289, in build_many
[dochtml]     raise worker_exc.original_exception
[dochtml] OSError: docstring of sage.symbolic.expression.Expression.subs:135: WARNING: Literal block expected; none found.
[dochtml] 
[dochtml]     Note: incremental documentation builds sometimes cause spurious
[dochtml]     error messages. To be certain that these are real errors, run
[dochtml]     "make doc-clean" first and try again.

comment:37 Changed 7 months ago by mkoeppe

Could someone fix up the documentation markup please?

Last edited 7 months ago by mkoeppe (previous) (diff)

comment:38 Changed 7 months ago by gh-DaveWitteMorris

I can't find any mistake, the patchbots are green, and the html documentation builds for me.

I wonder if maybe this error was posted to the wrong ticket? It is exactly the same error that was reported by a patchbot on ticket #30378 because I goofed up the WARNING block there:

[dochtml] OSError: docstring of sage.symbolic.expression.Expression.subs:135: WARNING: Literal block expected; none found.

comment:39 Changed 7 months ago by mkoeppe

  • Status changed from needs_work to positive_review

Thanks for checking! Works for me too (tested on top of 31344)

comment:40 Changed 6 months ago by vbraun

  • Branch changed from u/dimpase/packages/pynac/0727 to ccdf77cba4f57f2e8f595dbeefb2fb604d9f9ace
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:41 Changed 6 months ago by slelievre

  • Commit ccdf77cba4f57f2e8f595dbeefb2fb604d9f9ace deleted
  • Description modified (diff)
Note: See TracTickets for help on using tickets.