Opened 16 months ago
Closed 14 months ago
#31594 closed enhancement (fixed)
Accept giac1.7.x from the system
Reported by:  mjo  Owned by:  

Priority:  major  Milestone:  sage9.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: 
Description
This works fine outofthebox, we just need to tweak the version bounds in spkgconfigure.m4 to accept it.
Change History (27)
comment:1 Changed 16 months ago by
 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
comment:2 Changed 16 months ago by
 Milestone changed from sage9.3 to sage9.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 16 months ago by
I am prepared to accept this, although I probably would want #31563 to be done first.
comment:4 followup: ↓ 5 Changed 16 months ago by
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 16 months ago by
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 16 months ago by
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 16 months ago by
 Reviewers set to Dima Pasechnik, Travis Scrimshaw
 Status changed from needs_review to positive_review
comment:8 Changed 15 months ago by
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/sitepackages/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 15 months ago by
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 15 months ago by
evalb bug fixed https://dev.geogebra.org/trac/changeset/69847/ There are indeed some changes in symbolic integration and simplification in giac 1.7.03.
comment:11 Changed 15 months ago by
1.7.03 doesn't like the default std=c++17
of gcc11. 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 15 months ago by
 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 15 months ago by
Oh, they all return the same version number.
comment:14 followups: ↓ 15 ↓ 19 Changed 15 months ago by
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/sitepackages/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 15 months ago by
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 15 months ago by
The bump is trivial. c++17 is probably a can of worms.
comment:17 Changed 15 months ago by
By "fix" I mean appendcxxflags std=c++14
comment:18 Changed 15 months ago by
Yes, I did that in the overlay. A bit hamfisted but it basically works.
comment:19 in reply to: ↑ 14 ; followup: ↓ 20 Changed 15 months ago by
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/sitepackages/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 15 months ago by
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 15 months ago by
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 15 months ago by
I'm not blaming giac for anything here. The existing test was a bad choice:
 It was testing the exact string form of an indefinite integral, which, as you note, is unreliable for many reasons.
 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 followup: ↓ 25 Changed 15 months ago by
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#x230001.2
comment:24 Changed 15 months ago by
 Commit changed from 890c35414a729ec6e094ec2193b77da59290e705 to 55e36139c7719c838c68e0a4bf95472d709b1470
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
fa49b85  Trac #31594: accept giac1.7.x from the system.

d5d6689  Trac #31594: fix an interface test with newer versions of giac.

60a307a  Trac #31594: support giac1.7.x in symbolic integration tests.

55e3613  Trac #31594: simplify two libgiac simplify() examples.

comment:25 in reply to: ↑ 23 Changed 15 months ago by
 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#x230001.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:27 Changed 14 months ago by
 Branch changed from u/mjo/ticket/31594 to 55e36139c7719c838c68e0a4bf95472d709b1470
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Trac #31594: accept giac1.7.x from the system.