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

Upgrade giac to 1.9.0-15 #31563

Closed
mkoeppe opened this issue Mar 26, 2021 · 118 comments
Closed

Upgrade giac to 1.9.0-15 #31563

mkoeppe opened this issue Mar 26, 2021 · 118 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 26, 2021

https://www-fourier.ujf-grenoble.fr/~parisse/debian/dists/stable/main/source/

Previous update: #30537

Depends on #33226
Depends on #34037
Depends on #31403
Depends on #34088

CC: @orlitzky @kiwifb @antonio-rojas @frederichan-IMJPRG @sagetrac-parisse @sagetrac-tmonteil @dimpase

Component: packages: standard

Author: Matthias Koeppe

Branch: b4e5c6e

Reviewer: François Bissey

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

@mkoeppe mkoeppe added this to the sage-9.4 milestone Mar 26, 2021
@antonio-rojas
Copy link
Contributor

comment:1

FTR, this is straightforward and requires no changes to sagelib.

@orlitzky
Copy link
Contributor

orlitzky commented Apr 2, 2021

comment:4

I've updated the system version check in #31594 because it should be a trivial review.

@mkoeppe mkoeppe modified the milestones: sage-9.4, sage-9.5 Jul 19, 2021
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 2, 2021

Changed dependencies from #31562 to #32449

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 2, 2021

Branch: u/mkoeppe/upgrade_giac_to_1_7_0

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 2, 2021

Attachment: giac-1.7.0.25p0.tar.bz2.gz

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 2, 2021

Commit: 4df014a

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 2, 2021

comment:8

I have made a new tarball using spkg-src. Patches in build/pkgs/giac/patches/ still need updating/review, help welcome


New commits:

7d6a7b8Make test pass with giac 1.7
ffa2679Merge #32449
1127597build/pkgs/giac/patches/autotools/0001-configure.ac-Do-not-link-to-libintl-if-USE_NLS-is-no.patch: Remove, not needed for giac 1.7
35111c9build/pkgs/giac: Update to 1.7.0.25
4df014abuild/pkgs/giac/checksums.ini: Update upstream_url

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 3, 2021

comment:9

These are the current patches:
https://github.com/sagemath/sage-prod/tree/develop/build/pkgs/giac/patches
(I have already taken care of the ones in autotools/)

@kiwifb
Copy link
Member

kiwifb commented Sep 26, 2021

comment:10

I just tested the latest giac (1.7.0.33) against sage 9.5.beta2. The only doctests failing are

sage -t --long --random-seed=0 /usr/lib/python3.9/site-packages/sage/functions/min_max.py
too many failed tests, not using stored timings
Running doctests with ID 2021-09-27-12-21-06-b85d81e2.
Using --optional=dochtml,pip,sage
Doctesting 1 file.
sage -t --long --random-seed=0 /usr/lib/python3.9/site-packages/sage/functions/min_max.py
**********************************************************************
File "/usr/lib/python3.9/site-packages/sage/functions/min_max.py", line 236, in sage.functions.min_max.MaxSymbolic._evalf_
Failed example:
    r
Expected:
    sqrt(2) - cos(1)
Got:
    0.873911256505000
**********************************************************************
File "/usr/lib/python3.9/site-packages/sage/functions/min_max.py", line 238, in sage.functions.min_max.MaxSymbolic._evalf_
Failed example:
    r.n()
Expected:
    0.873911256504955
Got:
    0.873911256505000
**********************************************************************
1 item had failures:
   2 of  11 in sage.functions.min_max.MaxSymbolic._evalf_
    [69 tests, 2 failures, 0.53 s]

I'll note that recent giac unconditionally look for usb connecting libraries. It seems to be related to giac being able to go in embedded devices as far as I can tell. No real impact on the maths but the functionality is not clearly or cleanly separated.

@kiwifb
Copy link
Member

kiwifb commented Oct 18, 2021

comment:12

1.7.0.39 out. Did anyone notice that it was looking for curl before or is it recent?

dnl AC_CHECK_LIB(curl, main)
CONFIG_CURL="yes"
AC_ARG_ENABLE([curl],
	[AS_HELP_STRING([--enable-curl], [Use CURL [[default=yes]]])],
	[if test "$enableval" = "no"; then CONFIG_CURL="no"; fi], [])

if test "$CONFIG_CURL" = "yes"; then
	AC_CHECK_HEADER(curl/curl.h, [], [CONFIG_CURL="no"])
  fi
if test "$CONFIG_CURL" = "yes"; then
	AC_CHECK_LIB(curl, main, [], [CONFIG_CURL="no"])
  fi
AC_SUBST(CONFIG_CURL)
AC_SUBST(CURL_LIBS)
AM_CONDITIONAL(CONFIG_CURL, [test "$CONFIG_CURL" = "yes"])

It may have been there some time because it is an alternative to a code branch that can never be reached (pragma constant EMCC and EMCC2 are never defined anywhere).

@kiwifb
Copy link
Member

kiwifb commented Oct 18, 2021

comment:13

curl is in my dependency list in Gentoo, so I just didn't look properly the first _couple_ of times.

@sagetrac-parisse
Copy link
Mannequin

sagetrac-parisse mannequin commented Oct 19, 2021

comment:14

curl is used for the read command, if the argument is a string beginning with http.
For example read("https://www-fourier.univ-grenoble-alpes.fr/~parisse/numworks/py.py")

@kiwifb
Copy link
Member

kiwifb commented Oct 19, 2021

comment:15

Replying to @sagetrac-parisse:

curl is used for the read command, if the argument is a string beginning with http.
For example read("https://www-fourier.univ-grenoble-alpes.fr/~parisse/numworks/py.py")

That's certainly an interesting option. What about all that EMCC(2) code for emscript all around it that has no way of being enabled. Is it dead code or something in gestation?

@kiwifb
Copy link
Member

kiwifb commented Oct 19, 2021

comment:16

Replying to @kiwifb:

I just tested the latest giac (1.7.0.33) against sage 9.5.beta2. The only doctests failing are

sage -t --long --random-seed=0 /usr/lib/python3.9/site-packages/sage/functions/min_max.py
too many failed tests, not using stored timings
Running doctests with ID 2021-09-27-12-21-06-b85d81e2.
Using --optional=dochtml,pip,sage
Doctesting 1 file.
sage -t --long --random-seed=0 /usr/lib/python3.9/site-packages/sage/functions/min_max.py
**********************************************************************
File "/usr/lib/python3.9/site-packages/sage/functions/min_max.py", line 236, in sage.functions.min_max.MaxSymbolic._evalf_
Failed example:
    r
Expected:
    sqrt(2) - cos(1)
Got:
    0.873911256505000
**********************************************************************
File "/usr/lib/python3.9/site-packages/sage/functions/min_max.py", line 238, in sage.functions.min_max.MaxSymbolic._evalf_
Failed example:
    r.n()
Expected:
    0.873911256504955
Got:
    0.873911256505000
**********************************************************************
1 item had failures:
   2 of  11 in sage.functions.min_max.MaxSymbolic._evalf_
    [69 tests, 2 failures, 0.53 s]

This is really bizarre. min_max uses sympy not giac - at least directly. The only thing I can think of, is that giac is used to perform the integral and the result is numerical instead of symbolic. See the context of the failing tests

            sage: f = max_symbolic(sin(x), cos(x))
            sage: r = integral(f, x, 0, 1)
            ...
            sage: r
            sqrt(2) - cos(1)
            sage: r.n()
            0.873911256504955

@sagetrac-parisse
Copy link
Mannequin

sagetrac-parisse mannequin commented Oct 20, 2021

comment:17

Replying to @kiwifb:

Replying to @sagetrac-parisse:

curl is used for the read command, if the argument is a string beginning with http.
For example read("https://www-fourier.univ-grenoble-alpes.fr/~parisse/numworks/py.py")

That's certainly an interesting option. What about all that EMCC(2) code for emscript all around it that has no way of being enabled. Is it dead code or something in gestation?

It's not dead, it is enabled by commandline -D options, or by the compiler itself (emcc).
Try this [url=https://www-fourier.univ-grenoble-alpes.fr/%7eparisse/xcasen.html#exec&filename=%40session&python=1&radian=1&cas=0,0,read(%22https%3A%2F%2Fwww-fourier.univ-grenoble-alpes.fr%2F~parisse%2Fnumworks%2Fpy.py%22)&cas=0,200,go(f1)&cas=0,400,%3B&]session Xcas[/url]

@tornaria
Copy link
Contributor

comment:18

The test failure above seems to be a regression in giac 1.7.0:

  • giac 1.6.0 (as compiled by sage)
0>> int(max(sin(x),cos(x)), x, 0, 1)
Warning, integration of abs or sign assumes constant sign by intervals (correct if the argument is real):
Check [abs(sin(x)-cos(x))]
Discontinuities at zeroes of sin(x)-cos(x) were not checked
sqrt(2)-cos(1)
// Time 0.11

  • giac 1.7.0 (void linux)
0>> int(max(sin(x),cos(x)), x, 0, 1)
Piecewise definite integration: can only handle linear < or > condition
0.873911256505
// Time 0.01

Note that sympy_integrator is able to correctly evaluate this integral, but the order is maxima, then giac, then sympy.

sage: f = max_symbolic(sin(x), cos(x))
sage: sage.symbolic.integration.external.sympy_integrator(f, x, 0, 1)
sqrt(2) - cos(1)

@antonio-rojas
Copy link
Contributor

comment:19

Replying to @tornaria:

The test failure above seems to be a regression in giac 1.7.0:

Yes, more precisely it's a regression in 1.7.0-29. Since their forum is a gated community, it would be nice if someone with an account could report it.

@sagetrac-parisse
Copy link
Mannequin

sagetrac-parisse mannequin commented Nov 26, 2021

comment:20

This is a consequence of the fact that certified definite integration is hard. For this example Xcas session
there is only one point of discontinuity and it can easily be found by solving sin(x)=cos(x) on x=0..1, but for a large interval (say x=0..10000) or for a more complicated equation it's not the case. In order to make sure that I do not return a false answer, I have decided to return an exact answer only if I'm certain I did not miss a discontinuity in the antiderivative. It's certainly possible to improve, but not easy...

@tornaria
Copy link
Contributor

comment:21

OTOH, this still works on 1.7.0-39

0>> int((sin(x)+cos(x)+abs(cos(x)-sin(x)))/2, x, 0, 1)
Warning, integration of abs or sign assumes constant sign by intervals (correct if the argument is real):
Check [abs(cos(x)-sin(x))]
Discontinuities at zeroes of cos(x)-sin(x) were not checked
sqrt(2)-cos(1)
// Time 0.14

@tornaria
Copy link
Contributor

comment:22

Note that the warnings get through into sage

sage: integral((cos(x)+sin(x)+abs(cos(x)-sin(x)))/2, x, 0, 1)
Warning, integration of abs or sign assumes constant sign by intervals (correct if the argument is real):
Check [abs(cos(sageVARx)-sin(sageVARx))]
Discontinuities at zeroes of cos(sageVARx)-sin(sageVARx) were not checked
sqrt(2) - cos(1)

Would it be possible to integrate max(A,B) with a similar warning?

@tornaria
Copy link
Contributor

comment:23

BTW, did you notice that sage is reading giac output with less than 53-bit precision? We are getting 0.873911256505000 instead of 0.873911256504955, not because giac answer is incorrect but because its output is rounded to 0.873911256505 (which is correct rounding but less than 53-bit).

Is there a simple way to setup giac with more output precision so that sage gets the right answer correctly rounded?

@sagetrac-parisse
Copy link
Mannequin

sagetrac-parisse mannequin commented Nov 26, 2021

comment:24

max is translated internally to piecewise, it does not use the formula with max, and the checks differ.
If you run Digits:=14, you will get 14 digits printed. You can not get 53 bits of mantissa without multiprecision floats in giac, because the generic type of giac stores double in 8 bytes, and 5 bits are used to store the type -> the mantissa is rounded to 0 with 48 bits precision.

@tornaria
Copy link
Contributor

comment:25

Since there doesn't seem to be a fix available, I'm patching out the two failing doctests as follows:

--- a/src/sage/functions/min_max.py
+++ b/src/sage/functions/min_max.py
@@ -233,9 +233,9 @@ class MaxSymbolic(MinMax_base):
             sage: f = max_symbolic(sin(x), cos(x))
             sage: r = integral(f, x, 0, 1)
             ...
-            sage: r
+            sage: r             # not tested -- broken with giac 1.7.0
             sqrt(2) - cos(1)
-            sage: r.n()
+            sage: r.n()         # not tested -- broken with giac 1.7.0
             0.873911256504955
         """
         return max_symbolic(args)

I don't know if this is acceptable for including it in sagemath, but I can't downgrade giac and I rather skip this failing doctests. Is there a way to label a doctest as "expected fail" instead of "not tested" ?

@orlitzky
Copy link
Contributor

comment:26

Ironically, I think this test was added to showcase the fact that, given a difficult integral, sage would iterate through its available backends until it got an answer it liked. It seems now that giac is evolving towards the status quo, and refusing to give a possibly-incorrect exact answer.

We should still test the fallback behavior somehow, but these integrals have given us headaches for decades. I think we should stop promising to integrate them exactly, i.e. change/delete the tests.

@nasser1
Copy link

nasser1 commented Jul 9, 2022

comment:93

Replying to @mkoeppe:

on macOS:

[giac-1.9.0.15p0] /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/sys/cdefs.h:208:48: note: expanded from macro '__deprecated_msg'
[giac-1.9.0.15p0]         #define __deprecated_msg(_msg) __attribute__((__deprecated__(_msg)))
[giac-1.9.0.15p0]                                                       ^
[giac-1.9.0.15p0] icas.cc:1435:9: error: no member named 'localisation' in namespace 'xcas'
[giac-1.9.0.15p0]   xcas::localisation(); // change by L. Marohnić
[giac-1.9.0.15p0]   ~~~~~~^

FYI
I've reported this error xcas::localisation(); few days ago to giac forum. Here is the link

https://xcas.univ-grenoble-alpes.fr/forum/viewtopic.php?f=3&t=2789

--Nasser

@kiwifb
Copy link
Member

kiwifb commented Jul 9, 2022

comment:94

I didn't spot that one before because I always build with fltk support. And that part is in a "#elseblock when fltk is not requested. Blocks withxcas::localisation` have very funny guards

#ifndef USE_OBJET_BIDON
xcas::localisation(); // change by L. Marohnić
#endif

literally "use fake object". Passing -DUSE_OBJET_BIDON may help.

@kiwifb
Copy link
Member

kiwifb commented Jul 9, 2022

comment:95

Yes, I compiled it without fltk by adding -DUSE_OBJET_BIDON=1 to CPPFLAGS. Be careful "OBJET" without a "C", this is french and google translate will tell you that "bidon" is a "can", but colloquially it means "something that isn't what it seems", the can may look big but inside it is just empty.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 9, 2022

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

d2ade90build/pkgs/giac/spkg-install.in: Work around xcas::localisation error

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 9, 2022

Changed commit from 4160a64 to d2ade90

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 9, 2022

comment:97

Thanks both, this builds now

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 9, 2022

Changed dependencies from #33226, #34037, #31403 to #33226, #34037, #31403, #34088

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 9, 2022

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

d835afcbuild/pkgs/python3/distros/cygwin.txt: Add packages needed for ensurepip
d11f705Merge #34088

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 9, 2022

Changed commit from d2ade90 to d11f705

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 11, 2022

Changed commit from d11f705 to b4e5c6e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 11, 2022

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

b4e5c6eMerge tag '9.7.beta5' into t/31563/upgrade_giac_to_1_7_0

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 11, 2022

comment:103

Still fails on cygwin with same error as shown in comment:55 - https://github.com/mkoeppe/sage/runs/7274399777?check_suite_focus=true

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 11, 2022

comment:104

The other platforms looks OK.
I'd suggest that we ignore Cygwin for now and merge the ticket.

@kiwifb
Copy link
Member

kiwifb commented Jul 11, 2022

comment:105

Cygwin is low on my own priority list but it feels a bit of a shame. If giac 1.7 builds on cygwin this is clearly a regression, but while we upgrade the package here, there is nothing that prevent the use of 1.7 or specifically require 1.9, so if someone wants to build on cygwin, they can bring their own giac 1.7.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 11, 2022

comment:106

I think it will be easy to fix the Cygwin build failure in a follow up ticket. Best done in one go together with other current failures of the platform.

@kiwifb
Copy link
Member

kiwifb commented Jul 11, 2022

comment:107

I am OK with that plan. I am OK being a positive reviewer for this.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 11, 2022

comment:108

Thanks!

@kiwifb
Copy link
Member

kiwifb commented Jul 12, 2022

@vbraun
Copy link
Member

vbraun commented Jul 21, 2022

Changed branch from u/mkoeppe/upgrade_giac_to_1_7_0 to b4e5c6e

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 3, 2022

Changed commit from b4e5c6e to none

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 3, 2022

comment:111

Replying to @mkoeppe:

I think it will be easy to fix the Cygwin build failure in a follow up ticket. Best done in one go together with other current failures of the platform.

This is now #34269.

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

10 participants