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

Accept giac-1.7.x from the system #31594

Closed
orlitzky opened this issue Apr 2, 2021 · 30 comments
Closed

Accept giac-1.7.x from the system #31594

orlitzky opened this issue Apr 2, 2021 · 30 comments

Comments

@orlitzky
Copy link
Contributor

orlitzky commented Apr 2, 2021

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

CC: @mkoeppe @dimpase @isuruf @antonio-rojas @kiwifb

Component: build: configure

Author: Michael Orlitzky

Branch/Commit: 55e3613

Reviewer: Dima Pasechnik, Travis Scrimshaw

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

@orlitzky orlitzky added this to the sage-9.3 milestone Apr 2, 2021
@orlitzky
Copy link
Contributor Author

orlitzky commented Apr 2, 2021

Branch: u/mjo/ticket/31594

@orlitzky
Copy link
Contributor Author

orlitzky commented Apr 2, 2021

Commit: 890c354

@orlitzky
Copy link
Contributor Author

orlitzky commented Apr 2, 2021

New commits:

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

@mkoeppe
Copy link
Member

mkoeppe commented Apr 7, 2021

comment:2

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

@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.4 Apr 7, 2021
@tscrim
Copy link
Collaborator

tscrim commented Apr 11, 2021

comment:3

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

@orlitzky
Copy link
Contributor Author

comment:4

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 =)

@dimpase
Copy link
Member

dimpase commented Apr 14, 2021

comment:5

Replying to @orlitzky:

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 :-)

@dimpase
Copy link
Member

dimpase commented Apr 14, 2021

comment:6

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

@dimpase
Copy link
Member

dimpase commented Apr 14, 2021

Reviewer: Dima Pasechnik, Travis Scrimshaw

@antonio-rojas
Copy link
Contributor

comment:8

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

@antonio-rojas
Copy link
Contributor

comment:9

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?

@sagetrac-parisse
Copy link
Mannequin

sagetrac-parisse mannequin commented May 2, 2021

comment:10

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.

@kiwifb
Copy link
Member

kiwifb commented May 2, 2021

comment:11

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.

@orlitzky
Copy link
Contributor Author

orlitzky commented May 9, 2021

comment:12

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

@orlitzky
Copy link
Contributor Author

orlitzky commented May 9, 2021

comment:13

Oh, they all return the same version number.

@antonio-rojas
Copy link
Contributor

comment:14

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*(...)

@orlitzky
Copy link
Contributor Author

orlitzky commented May 9, 2021

comment:15

Replying to @antonio-rojas:

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.

@kiwifb
Copy link
Member

kiwifb commented May 9, 2021

comment:16

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

@orlitzky
Copy link
Contributor Author

orlitzky commented May 9, 2021

comment:17

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

@kiwifb
Copy link
Member

kiwifb commented May 9, 2021

comment:18

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

@orlitzky
Copy link
Contributor Author

comment:19

Replying to @antonio-rojas:

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.

@sheerluck
Copy link
Contributor

comment:20

Replying to @orlitzky:

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

@sagetrac-parisse
Copy link
Mannequin

sagetrac-parisse mannequin commented May 10, 2021

comment:21

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.

@orlitzky
Copy link
Contributor Author

comment:22

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.

@sagetrac-parisse
Copy link
Mannequin

sagetrac-parisse mannequin commented May 10, 2021

comment:23

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 11, 2021

Changed commit from 890c354 to 55e3613

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 11, 2021

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.

@orlitzky
Copy link
Contributor Author

comment:25

Replying to @sagetrac-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.

@dimpase
Copy link
Member

dimpase commented May 11, 2021

comment:26

lgtm

@vbraun
Copy link
Member

vbraun commented Jun 6, 2021

Changed branch from u/mjo/ticket/31594 to 55e3613

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

8 participants