update giac to 1.5.0-63

dimpase commented 5 years ago

update giac to the latest version - probably related to the problem on #27385

from the source at www-fourier.ujf-grenoble.fr/~parisse/debian/dists/stable/main/source/giac_1.5.0-63.*

produced tarball: http://users.ox.ac.uk/~coml0531/sage/giac-1.5.0.63.tar.bz2

Upstream: Fixed upstream, in a later stable release.

CC: @embray @kiwifb @sagetrac-parisse

Component: packages: standard

Author: Dima Pasechnik

Branch/Commit: 132f560

Reviewer: François Bissey

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

dimpase commented 5 years ago

Description changed:

--- 
+++ 
@@ -1 +1,3 @@
 update giac to the latest version - probably related to the problem on #27385
+
+The source tarball: http://www-fourier.ujf-grenoble.fr/~parisse/debian/dists/stable/main/source/giac_1.5.0-61.tar.gz

dimpase commented 5 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,5 @@
 update giac to the latest version - probably related to the problem on #27385

-The source tarball: http://www-fourier.ujf-grenoble.fr/~parisse/debian/dists/stable/main/source/giac_1.5.0-61.tar.gz
+from the source at www-fourier.ujf-grenoble.fr/~parisse/debian/dists/stable/main/source/giac_1.5.0-61.*
+
+produced tarball: http://users.ox.ac.uk/~coml0531/sage/giac-1.5.0.61.tar.bz2

dimpase commented 5 years ago

Commit: 277e1d9

dimpase commented 5 years ago

Branch: u/dimpase/packages/giac15061

dimpase commented 5 years ago

Author: Dima Pasechnik

dimpase commented 5 years ago

New commits:

277e1d9 update giac to version 1.5.0-61

kiwifb commented 5 years ago

comment:5

I meant to post something about that.

This release in particular breaks the building of giacpy_sage the following patch https://github.com/cschwan/sage-on-gentoo/blob/master/sci-mathematics/giac/files/giac-1.5.0.61-tex_header.patch fixes it. You are welcome to forward it upstream. Note that this is the only instance of #include <tex.h> all the others (and there is number of them) are "tex.h". I don't think that's particularly good style but it works.

For a few releases of giac a giacpy_sage doctest has been broken.

sage -t --long /usr/lib64/python2.7/site-packages/sage/libs/giac.py
**********************************************************************
File "/usr/lib64/python2.7/site-packages/sage/libs/giac.py", line 209, in sage.libs.giac.?
Failed example:
    B1 = gb_giac(I.gens(),1e-16) # optional - giacpy_sage, long time (1s)
Expected:
    Running a probabilistic check for the reconstructed Groebner basis.
    If successfull, error probability is less than 1e-16 ...
Got:
    0.338649 adding reconstructed ideal generators 18 (reconpart 0.233766 prev 0 augment 0.2 recon_count 8 th 1 recon_n2 18 V[i] 77)
    0.339567 # new ideal generators 23
    0.567448 adding reconstructed ideal generators 36 (reconpart 0.467532 prev 0.233766 augment 0.2 recon_count 17 th 1 recon_n2 36 V[i] 77)
    0.57095 # new ideal generators 41
    0.725683 adding reconstructed ideal generators 52 (reconpart 0.675325 prev 0.467532 augment 0.2 recon_count 11 th 1 recon_n2 52 V[i] 77)
    0.729126 # new ideal generators 57
    Running a probabilistic check for the reconstructed Groebner basis. If successfull, error probability is less than 1e-16 and is estimated to be less than 10^-63. Use proba_epsilon:=0 to certify (this takes more time).
    // Groebner basis computation time 0.680109 Memory 212.216M
**********************************************************************

Just a lot more verbose.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

1fda15e use "tex.h" rather than in #include

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 277e1d9 to 1fda15e

dimpase commented 5 years ago

Upstream: Reported upstream. No feedback yet.

kiwifb commented 5 years ago

comment:8

What about the giacpy_sage doctest?

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

0be07f5 more verbosity in a long optional test

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 1fda15e to 0be07f5

dimpase commented 5 years ago

comment:10

OK, done (I forgot it's a long test that's broken...)

kiwifb commented 5 years ago

comment:11

LGTM now.

kiwifb commented 5 years ago

Reviewer: François Bissey

mwageringel commented 5 years ago

comment:12

I can confirm that this update seems to solve the problem I described in #27385 on Linux. If I understand correctly, this should also solve the problem on Cygwin. If so, the cygwin-icas.patch should be deleted, should it not?

dimpase commented 5 years ago

comment:13

Unless it is tested on Cygwin, I won't rush. It's never late to delete a Cygwin-specific patch on another ticket.

embray commented 5 years ago

comment:14

It would be nice for me to be able to see exactly what changed in the new giac version specifically to fix this problem, so that I can confirm that the problem as I understood it does appear fixed. Unfortunately that's difficult without a publicly viewable change history...

dimpase commented 5 years ago

comment:15

Replying to @embray:

It would be nice for me to be able to see exactly what changed in the new giac version specifically to fix this problem, so that I can confirm that the problem as I understood it does appear fixed. Unfortunately that's difficult without a publicly viewable change history...

The history is embedded in geogebra SVN, cf e.g. history/trac

Perhaps Bernard can point at the right ticket on geogebra's trac.

2edf2d52-6c17-4f2a-88a6-e860db4dcddf commented 5 years ago

comment:16

This change is not in SVN geogebra, because it's UI code (Xcas code), not Giac code. I have added if (status!=1) in the non-FLTK code, like in the FLTK code

#ifdef HAVE_LIBFLTK
    if (status!=1)
      xcas::Xcas_debugguer(status,contextptr);
#else
    // FIXME Debugguer without FLTK
    if (status!=1)
      giac::thread_eval_status(1,contextptr);
#endif

It would be a good idea to check the resulting binary anyway. BTW, I will probably update giac to 1.5.0-63 next week, with a small header fix reported by dimpase.

dimpase commented 5 years ago

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

dimpase commented 5 years ago

comment:18

Replying to @sagetrac-parisse:

This change is not in SVN geogebra, because it's UI code (Xcas code), not Giac code.

Is there a VCS repo somewhere with these Xcas changes? Without such a repo it's quite hard to follow up the changes.

2edf2d52-6c17-4f2a-88a6-e860db4dcddf commented 5 years ago

comment:19

No, because Xcas GUI code is not used by other projects. Geogebra has it's own version of a very simple commandline interface (for testing purpose), maybe it would be sufficient for sage as well or any adaptation: https://dev.geogebra.org/trac/browser/trunk/geogebra/giac/src/minigiac/cpp

dimpase commented 5 years ago

comment:20

Replying to @sagetrac-parisse:

No, because Xcas GUI code is not used by other projects.

The current thread is about an issue in Xcas that affects Sagemath. It is a proof that Xcas is used in other projects. If Xcas was on a public VCS then it's very likely that one could merely look up the relevant change there, instead of having this discussion.

embray commented 5 years ago

comment:21

Replying to @dimpase:

Replying to @sagetrac-parisse:

No, because Xcas GUI code is not used by other projects.

The current thread is about an issue in Xcas that affects Sagemath. It is a proof that Xcas is used in other projects.

I think Bernard's point here (which I agree with, if I understand correctly) is that the original bug I found was caused by multi-threaded evaluation code that is needed by Xcas (so that giac commands can be evaluated in a thread without blocking the UI), but that is not needed by non-GUI interfaces.

The problem is that the threaded eval code had a bug, but it also wasn't even needed in the context where Sage was using it, so I just disabled use of the threaded-eval in the case where it was affecting me.

The code in comment:16 does appear to be addressing the bug I described in #27385 comment:14

(I still think the code should be using proper condition primitives but that's a bigger change; the above fix will definitely reduce the likelihood of a problem here).

2edf2d52-6c17-4f2a-88a6-e860db4dcddf commented 5 years ago

comment:22

Yes, I believe it would be simpler and more robust if Sage interface was with a Sage specific giac commandline program (like the one Geogebra is using for regression testing), and not with Xcas's icas that is designed and tested to run with FLTK.

kiwifb commented 5 years ago

comment:23

1.5.0-63 has been out for a few days and has the header fix.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 0be07f5 to 132f560

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

132f560 update giac to 1.5.0-63

dimpase commented 5 years ago

Description changed:

--- 
+++ 
@@ -1,5 +1,5 @@
 update giac to the latest version - probably related to the problem on #27385

-from the source at www-fourier.ujf-grenoble.fr/~parisse/debian/dists/stable/main/source/giac_1.5.0-61.*
+from the source at www-fourier.ujf-grenoble.fr/~parisse/debian/dists/stable/main/source/giac_1.5.0-63.*

-produced tarball: http://users.ox.ac.uk/~coml0531/sage/giac-1.5.0.61.tar.bz2
+produced tarball: http://users.ox.ac.uk/~coml0531/sage/giac-1.5.0.63.tar.bz2

kiwifb commented 5 years ago

comment:27

Completely happy with this.

vbraun commented 5 years ago

Changed branch from u/dimpase/packages/giac15061 to 132f560

sagemath / sage

update giac to 1.5.0-63 #28101