Opened 6 months ago

Closed 4 months ago

#31594 closed enhancement (fixed)

Accept giac-1.7.x from the system

Reported by: mjo Owned by:
Priority: major Milestone: sage-9.4
Component: build: configure Keywords:
Cc: mkoeppe, dimpase, isuruf, arojas, fbissey Merged in:
Authors: Michael Orlitzky Reviewers: Dima Pasechnik, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 55e3613 (Commits, GitHub, GitLab) Commit: 55e36139c7719c838c68e0a4bf95472d709b1470
Dependencies: Stopgaps:

Status badges

Description

This works fine out-of-the-box, we just need to tweak the version bounds in spkg-configure.m4 to accept it.

Change History (27)

comment:1 Changed 6 months ago by mjo

  • Branch set to u/mjo/ticket/31594
  • Cc mkoeppe dimpase isuruf arojas fbissey added
  • Commit set to 890c35414a729ec6e094ec2193b77da59290e705
  • Status changed from new to needs_review

New commits:

890c354Trac #31594: accept giac-1.7.x from the system.

comment:2 Changed 5 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

Sage development has entered the release candidate phase for 9.3. Setting a new milestone for this ticket based on a cursory review.

comment:3 Changed 5 months ago by tscrim

I am prepared to accept this, although I probably would want #31563 to be done first.

comment:4 follow-up: Changed 5 months ago by mjo

One of the main benefits of using system packages is that we don't all have to wait for someone to duplicate pointless work in the SPKG =)

comment:5 in reply to: ↑ 4 Changed 5 months ago by dimpase

Replying to mjo:

One of the main benefits of using system packages is that we don't all have to wait for someone to duplicate pointless work in the SPKG =)

I am impatiently waiting for removal of gcc and gfortran packages (along with the rest of Sage toolchain), to start with :-)

comment:6 Changed 5 months ago by dimpase

let me test on Gentoo. It seems I'd also need pari 2.13 from the system for this to work, though.

comment:7 Changed 5 months ago by dimpase

  • Reviewers set to Dima Pasechnik, Travis Scrimshaw
  • Status changed from needs_review to positive_review

comment:8 Changed 5 months ago by arojas

Beware that the latest 1.7.0.3 version causes some breakage. Some tests fail because of changes in expression expansion, but there are some other weird failures such as

File "/usr/lib/python3.9/site-packages/sage/interfaces/giac.py", line 724, in sage.interfaces.giac.Giac._equality_symbol
Failed example:
    giac(2) == giac(2)
Expected:
    True
Got:
    False

comment:9 Changed 5 months ago by arojas

Seems that evalb is just broken in 1.7.0.3

0>> a:=0
0
// Time 0
1>> evalb(a==0)
false
// Time 0

Could someone with an account please report it upstream?

comment:10 Changed 5 months ago by parisse

evalb bug fixed https://dev.geogebra.org/trac/changeset/69847/ There are indeed some changes in symbolic integration and simplification in giac 1.7.0-3.

comment:11 Changed 5 months ago by fbissey

1.7.0-3 doesn't like the default -std=c++17 of gcc-11. I guess that applies to all the previous versions as well. Setting CXX="g++ -std=c++14" or passing the standard as a CXXFLAGS does work.

comment:12 Changed 4 months ago by mjo

  • Status changed from positive_review to needs_work

I'll change this to accept only 1.7.0.1 for now, since the simplification changes are likely to stick around.

comment:13 Changed 4 months ago by mjo

Oh, they all return the same version number.

comment:14 follow-ups: Changed 4 months ago by arojas

Why not just modify the tests so they pass with both versions? IMHO a slight change in output format shouldn't block using a system package. And some of these tests already have a very generic expected output:

File "/usr/lib/python3.9/site-packages/sage/interfaces/giac.py", line 609, in sage.interfaces.giac.Giac._eval_line
Failed example:
    giac(h)
Expected:
    12*(...)

comment:15 in reply to: ↑ 14 Changed 4 months ago by mjo

Replying to arojas:

Why not just modify the tests so they pass with both versions?

I came to this same conclusion a few seconds ago =)

First I have to update our Gentoo package to 1.7.0.5 so that I can see the new output and fix the c++17 issue, but then I'll update this branch with tests that work with all versions.

comment:16 Changed 4 months ago by fbissey

The bump is trivial. c++17 is probably a can of worms.

comment:17 Changed 4 months ago by mjo

By "fix" I mean append-cxxflags -std=c++14

comment:18 Changed 4 months ago by fbissey

Yes, I did that in the overlay. A bit ham-fisted but it basically works.

comment:19 in reply to: ↑ 14 ; follow-up: Changed 4 months ago by mjo

Replying to arojas:

Why not just modify the tests so they pass with both versions? IMHO a slight change in output format shouldn't block using a system package. And some of these tests already have a very generic expected output:

File "/usr/lib/python3.9/site-packages/sage/interfaces/giac.py", line 609, in sage.interfaces.giac.Giac._eval_line
Failed example:
    giac(h)
Expected:
    12*(...)

This is not such a great example anyway:

sage: f = 1/x*((-2*x^(1/3)+1)^(1/4))^3                                                                                                                                       
sage: integrate(f,x,algorithm='maxima')(x=1).n()                                                                                                                             
-0.760158905420130 + 10.1849368661895*I
sage: integrate(f,x,algorithm='sympy')(x=1).n()                                                                                                                              
-10.1849368661895 + 10.1849368661895*I
sage: integrate(f,x,algorithm='giac')(x=1).n()                                                                                                                               
-0.760158905420130 + 4.29445064070865*I

I think I'll replace it with an integral that I can do in my head.

comment:20 in reply to: ↑ 19 Changed 4 months ago by gh-sheerluck

Replying to mjo:

sage: f = 1/x*((-2*x^(1/3)+1)^(1/4))^3                                                                                                                                       
sage: integrate(f,x,algorithm='maxima')(x=1).n()                                                                                                                             
-0.760158905420130 + 10.1849368661895*I
sage: integrate(f,x,algorithm='sympy')(x=1).n()                                                                                                                              
-10.1849368661895 + 10.1849368661895*I
sage: integrate(f,x,algorithm='giac')(x=1).n()                                                                                                                               
-0.760158905420130 + 4.29445064070865*I
sage: integrate(f,x,algorithm='fricas')(x=1).n()
-0.760158905420130 + 10.1849368661895*I

comment:21 Changed 4 months ago by parisse

If I understand correctly, you are evaluating an antiderivative at x=1 and you get an approximation. I do not understand how this test can be meaningful, since an antiderivative is defined up to a constant. One could argue it should stay the same for a given CAS, but that's not true, I made some improvements in symbolic integration recently in giac, and that means sometimes that the algorithm changes and the antiderivative expression too.

comment:22 Changed 4 months ago by mjo

I'm not blaming giac for anything here. The existing test was a bad choice:

  1. It was testing the exact string form of an indefinite integral, which, as you note, is unreliable for many reasons.
  2. The results of that indefinite integral do not agree (even up to a constant) between the various integration engines we have, so who knows if "expected" output is correct.

My use of (x=1).n() just makes it clear that the answers are different, since the resulting expression is rather ugly.

comment:23 follow-up: Changed 4 months ago by parisse

A more meaningful test would be F=integrate(f) then simplify(diff(F)-f). I have added some examples in the check subdirectory of giac, from the independent integrals testsuite from https://www.12000.org/my_notes/CAS_integration_tests/reports/rubi_4_16_1_graded/inch1.htm#x2-30001.2

comment:24 Changed 4 months ago by git

  • Commit changed from 890c35414a729ec6e094ec2193b77da59290e705 to 55e36139c7719c838c68e0a4bf95472d709b1470

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

fa49b85Trac #31594: accept giac-1.7.x from the system.
d5d6689Trac #31594: fix an interface test with newer versions of giac.
60a307aTrac #31594: support giac-1.7.x in symbolic integration tests.
55e3613Trac #31594: simplify two libgiac simplify() examples.

comment:25 in reply to: ↑ 23 Changed 4 months ago by mjo

  • Status changed from needs_work to needs_review

Replying to parisse:

A more meaningful test would be F=integrate(f) then simplify(diff(F)-f). I have added some examples in the check subdirectory of giac, from the independent integrals testsuite from https://www.12000.org/my_notes/CAS_integration_tests/reports/rubi_4_16_1_graded/inch1.htm#x2-30001.2

The call to simplify() takes ages in this case. Since the point of that test is to check that we can pass a string to giac(), I changed the example to sin(x)^2 + cos(x)^2 and have compared the result (x) as a symbolic expression.

comment:26 Changed 4 months ago by dimpase

  • Status changed from needs_review to positive_review

lgtm

comment:27 Changed 4 months ago by vbraun

  • Branch changed from u/mjo/ticket/31594 to 55e36139c7719c838c68e0a4bf95472d709b1470
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.