#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:  sage9.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: 
Description (last modified by )
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 2 years ago by
comment:2 Changed 2 years ago by
Owner:  set to ghbmlivin 

comment:3 Changed 2 years ago by
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 2 years ago by
I can confirm this on Ubuntu 18 with SageMath version 9.2.beta13, using Python 3.8.5.
comment:5 Changed 2 years ago by
Owner:  ghbmlivin deleted 

Report Upstream:  N/A → 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 2 years ago by
Authors:  → Ben Livingston 

Report Upstream:  Not yet reported upstream; Will do shortly. → Fixed upstream, in a later stable release. 
Status:  new → 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 2 years ago by
Milestone:  sage9.2 → sage9.3 

comment:8 Changed 2 years ago by
OK, trying https://patchdiff.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 2 years ago by
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 2 years ago by
Branch:  → u/dimpase/packages/pynac/30446 

Commit:  → dc073dfa9224efb61dc01d49ef60a9c7fa0dc568 
Reviewers:  → Dima Pasechnik 
Status:  needs_review → positive_review 
patches from pynac PRs to be removed at the next pynac update.
New commits:
dc073df  fixes for trac #30446

comment:11 Changed 2 years ago by
comment:12 Changed 2 years ago by
fyi: Patchbot Linux/74Ubuntu_SMP_Tue_Sep_17_17%3A06%3A04_UTC_2019/x86_64/4.15.065generic/pc72
seems to have failed to build pynac at ticket #30786. ("Error installing package pynac0.7.26.sage20200403.p1") If there is a genuine problem, I think it must be because of this ticket.
comment:14 Changed 2 years ago by
fyi: Another ubuntu patchbot failed to build pynac (on ticket #31137 this time). Both failures say WARNING: 'aclocal1.16' is missing on your system. ... Makefile:432: recipe for target 'aclocal.m4' failed
.
comment:15 Changed 2 years ago by
Status:  positive_review → needs_work 

Patch touches configure stuff which triggers automake run (which is not a Sage dependency)
Building pynac0.7.26.sage20200403.p1 CDPATH="${ZSH_VERSION+.}:" && cd . && /bin/bash /var/lib/buildbot/slave/sage_git/build/local/var/tmp/sage/build/pynac0.7.26.sage20200403.p1/src/missing aclocal1.16 I m4 /var/lib/buildbot/slave/sage_git/build/local/var/tmp/sage/build/pynac0.7.26.sage20200403.p1/src/missing: line 81: aclocal1.16: command not found WARNING: 'aclocal1.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 2 years ago by
Priority:  major → critical 

I set this to critical since many tickets (some already positively reviewed providing a doctest) rely on this one.
comment:17 Changed 2 years ago by
https://doc.sagemath.org/html/en/developer/packaging.html#whentopatchwhentorepackagewhentoautoconfiscate 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 2 years ago by
Dependencies:  → #31118 

I'll repurpose this ticket for the package update.
comment:19 Changed 2 years ago by
Branch:  u/dimpase/packages/pynac/30446 → u/dimpase/packages/pynac/0727 

Commit:  dc073dfa9224efb61dc01d49ef60a9c7fa0dc568 → 563940e8554fe96a16fdf669c03f895aacf27b3c 
Report Upstream:  Fixed upstream, in a later stable release. → None of the above  read trac for reasoning. 
Status:  needs_work → needs_review 
New commits:
9d747fc  sage package updatelatest: Distinguish pypi package name and spkg name

f74f66c  sage package updatelatest: Accept package classes :standard:, :optional: etc., restrict to normal Python packages

182b3d2  sage package fixchecksum: Handle package classes, ignore nonnormal packages

9a57cf6  pynac update

563940e  doctest from #30446

comment:20 followup: 26 Changed 2 years ago by
When you make official releases in https://github.com/pynac/pynac, there is no need for a version suffix such as "sage20200403". 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 2 years ago by
Status:  needs_review → needs_work 

comment:22 Changed 2 years ago by
Commit:  563940e8554fe96a16fdf669c03f895aacf27b3c → 51def3cc9203861a11404019d68ed90937d9f017 

Branch pushed to git repo; I updated commit sha1. New commits:
51def3c  sane version name

comment:23 Changed 2 years ago by
Commit:  51def3cc9203861a11404019d68ed90937d9f017 → 4834dc8a7ab300c57921986e847972030b0547ca 

Branch pushed to git repo; I updated commit sha1. New commits:
4834dc8  remove upstreamed patch

comment:24 Changed 2 years ago by
Status:  needs_work → needs_review 

comment:25 Changed 2 years ago by
Status:  needs_review → needs_work 

should provide a cleaner tarball
comment:26 Changed 2 years ago by
Replying to mkoeppe:
When you make official releases in https://github.com/pynac/pynac, there is no need for a version suffix such as "sage20200403". 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 2 years ago by
Commit:  4834dc8a7ab300c57921986e847972030b0547ca → 214712de4fd44f9e703cfffc7cccced7b5757a12 

Branch pushed to git repo; I updated commit sha1. New commits:
214712d  tarball update

comment:28 Changed 2 years ago by
Authors:  Ben Livingston → Ben Livingston, Dima Pasechnik 

Status:  needs_work → needs_review 
comment:29 Changed 2 years ago by
build/pkgs/pynac/SPKG.rst
could use an update. In particular "Special Update/Build? Instructions" should be removed
comment:30 Changed 2 years ago by
Reviewers:  Dima Pasechnik → Dima Pasechnik, Matthias Koeppe 

comment:31 Changed 2 years ago by
Status:  needs_review → positive_review 

comment:32 Changed 2 years ago by
Summary:  wrong result on symbolic exponentiation → Update pynac to 0.7.27 to fix wrong result on symbolic exponentiation 

comment:33 Changed 2 years ago by
Commit:  214712de4fd44f9e703cfffc7cccced7b5757a12 → ccdf77cba4f57f2e8f595dbeefb2fb604d9f9ace 

Status:  positive_review → needs_review 
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
ccdf77c  update SPKG.rst

comment:35 Changed 2 years ago by
Cc:  zimmerma added 

comment:36 Changed 2 years ago by
Status:  positive_review → 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/sitepackages/sage_setup/docbuild/__main__.py", line 2, in <module> [dochtml] main() [dochtml] File "/home/release/Sage/local/lib64/python3.9/sitepackages/sage_setup/docbuild/__init__.py", line 1730, in main [dochtml] builder() [dochtml] File "/home/release/Sage/local/lib64/python3.9/sitepackages/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/sitepackages/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/sitepackages/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/sitepackages/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/sitepackages/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 docclean" first and try again.
comment:38 Changed 2 years ago by
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 2 years ago by
Status:  needs_work → positive_review 

Thanks for checking! Works for me too (tested on top of 31344)
comment:40 Changed 2 years ago by
Branch:  u/dimpase/packages/pynac/0727 → ccdf77cba4f57f2e8f595dbeefb2fb604d9f9ace 

Resolution:  → fixed 
Status:  positive_review → closed 
comment:41 Changed 2 years ago by
Commit:  ccdf77cba4f57f2e8f595dbeefb2fb604d9f9ace 

Description:  modified (diff) 
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.)