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

make the experimental FriCAS package optional #23847

Closed
mantepse opened this issue Sep 13, 2017 · 67 comments
Closed

make the experimental FriCAS package optional #23847

mantepse opened this issue Sep 13, 2017 · 67 comments

Comments

@mantepse
Copy link
Contributor

FriCAS is a CAS that provides some functionality which is otherwise either missing (e.g., guessing formulas), or not as complete as might be desirable (e.g., symbolic integration).

The FriCAS interface was completely rewritten after FriCAS was demoted to experimental in #18563 and is now quite mature, containing lots of doctests.

Depends on #21377
Depends on #22525
Depends on #23782

CC: @fchapoton @EmmanuelCharpentier @embray

Component: packages: optional

Keywords: FriCAS

Author: Martin Rubey

Branch/Commit: f48e8c5

Reviewer: Emmanuel Charpentier, Jeroen Demeyer, Dima Pasechnik, Erik Bray

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

@mantepse
Copy link
Contributor Author

Changed keywords from none to FriCAS

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented Sep 27, 2017

comment:3

Replying to @mantepse:

What kind of review do you think is suitable for this ? I have had fricas installed for about 2 years, with no specific problems.

Since, AFAICT, #21377 and #22525 are already merged in 8.1.beta6, the only test I could do would be to test #23782 and ptestlong.

Maybe we should grasp the opportunity to document this interface a bit better, and possibly doctest some opportunities made available by fricas ?

In your post, you quoted "guessing formulas", lazy powerseries, as well as enhancements to symbolic integration and differential equations solving. Documenting those examples, either in the relevant "main" documentation or in a specific page) and doctesting them would be useful.

What do you think ?

@mantepse
Copy link
Contributor Author

comment:5

In your post, you quoted "guessing formulas", lazy powerseries, as well as enhancements to symbolic integration and differential equations solving. Documenting those examples, either in the relevant "main" documentation or in a specific page) and doctesting them would be useful.

These are actually in the main documentation. To see it:

sage: import sage.interfaces.fricas
sage: sage.interfaces.fricas?

I agree that exposing some of the functionality would be good, but I think that's for another ticket. I wouldn't do it for an experimental package.

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented Sep 27, 2017

comment:6

Replying to @mantepse:

[ Snip... ]

I agree that exposing some of the functionality would be good, but I think that's for another ticket.

You may be right...

I wouldn't do it for an experimental package.

But your ticket aims to make it an optional package...

Anyway : sage 8.1.beta6+#23782 passes ptestlong without any error. What remains to do is changing $SAGE_ROOT/build/pkgs/fricas/type from experimental to optional, fix the checksums, commit and push. Care to do it ? (I can review it this afternoon...).

@mantepse
Copy link
Contributor Author

comment:7

I wouldn't do it for an experimental package.

But your ticket aims to make it an optional package...

Yes, I might do it for an optional package :-)

What remains to do is changing $SAGE_ROOT/build/pkgs/fricas/type from experimental to optional, fix the checksums, commit and push. Care to do it ? (I can review it this afternoon...).

why would the checksum change? (fricas was updated in #21377)

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented Sep 27, 2017

comment:8

Replying to @mantepse:

I wouldn't do it for an experimental package.

But your ticket aims to make it an optional package...

Yes, I might do it for an optional package :-)

What remains to do is changing $SAGE_ROOT/build/pkgs/fricas/type from experimental to optional, fix the checksums, commit and push. Care to do it ? (I can review it this afternoon...).

why would the checksum change? (fricas was updated in #21377)

'Cause, unless I'm mistaken, the $SAGE_ROOT/build/pkgs/fricas/type file is accoundd for in the checksum computation ?

@jdemeyer
Copy link

comment:9

It would be easier to test this if all dependencies would actually be merged in a beta release.

@jdemeyer
Copy link

comment:10

Replying to @EmmanuelCharpentier:

'Cause, unless I'm mistaken, the $SAGE_ROOT/build/pkgs/fricas/type file is accoundd for in the checksum computation ?

No, the checksum is the checksum of the source tarball. It does not depend on anything from Sage.

@jdemeyer
Copy link

comment:11

About this ticket itself: the way I see it, the only difference between an optional package and an experimental package is that an optional package is required to work on all supported platforms. "working" is defined as building, passing its own testsuite and passing Sage doctests. Regarding the test suite: does FriCAS have one? There is no spkg-check file.

If it works as defined above, then there doesn't even need to be a vote: just make it optional.

I can run some tests on various systems. As I said above, I'd prefer to wait for a beta release where are dependencies have been merged.

@mantepse
Copy link
Contributor Author

comment:12

Replying to @jdemeyer:

About this ticket itself: the way I see it, the only difference between an optional package and an experimental package is that an optional package is required to work on all supported platforms.
"working" is defined as building, passing its own testsuite and passing Sage doctests.

One of the FriCAS maintainers says (https://groups.google.com/d/msg/fricas-devel/s5WtJx8u-kA/XVJPuUAQBwAJ) that this should be the case. I have no way of testing it.

Regarding the test suite: does FriCAS have one? There is no spkg-check file.

Concerning this, he answered:

You can invoke the testsuite by "cd src/input && make", although there's no script to collect the results, there's an unofficial one:

https://raw.githubusercontent.com/oldk1331/fricas/116f64b1fea32b7148421a4efb43117cf3eee819/src/scripts/test.sh

I wonder how useful this testsuite is for sage then. I also do not know how long it takes to run it. I just started it.

If it works as defined above, then there doesn't even need to be a vote: just make it optional.

I can run some tests on various systems. As I said above, I'd prefer to wait for a beta release where are dependencies have been merged.

I'll set the type to optional and the ticket to needs-review and you do it whenever you wish, OK?

@mantepse
Copy link
Contributor Author

@mantepse
Copy link
Contributor Author

comment:14

On my laptop (Intel(R) Celeron(R) CPU N2940 @ 1.83GHz) the test suite runs for about half an hour. For each of the ~ 200 input files it produces an output file. Essential differences mean new bugs.

I don't think that we want to run this.


New commits:

f8567ebset type to optional

@mantepse
Copy link
Contributor Author

Commit: f8567eb

@jdemeyer
Copy link

comment:15

Replying to @mantepse:

I don't think that we want to run this.

Why not? I don't understand how you came to this conclusion.

@mantepse
Copy link
Contributor Author

comment:16

Replying to @jdemeyer:

Replying to @mantepse:

I don't think that we want to run this.

Why not? I don't understand how you came to this conclusion.

Because you don't get a result, such as "tests passed". Well, it might make sense to run it and say something like "output of tests can be found in ..."

What do you think?

@jdemeyer
Copy link

comment:17

Replying to @mantepse:

Because you don't get a result, such as "tests passed".

OK, got it.

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented Oct 4, 2017

comment:19

LGTM : on Debian testing running on core i7 + 16 GB RAM, 8.1-beta7+#23847 passes ptestlong with no error whatsoever (mirabile dictu), as well as sage -t --optional fricas,sage src/sage/interfaces/fricas.py.

As suggested by Jeroen Demeyer, I think that this should be tested on other "officially supported Sage platforms", whatever that means.

At the very minimum, I suggest Mac OS X (which is full of surprises, nowadays), the Sage appliance used under Windows, and maybe the Cygwin(-64 ?) port made by Erik Bray, if Erik thinks that his port is ready for diffusion (I put him in Cc).

Status remains at needs_review, but should be put as positive_review when this will have been tested on all "interesting" platforms.

@jdemeyer
Copy link

jdemeyer commented Oct 4, 2017

comment:20

All dependencies have now been merged in 8.1.beta7

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented Oct 4, 2017

comment:21

Replying to @jdemeyer:

All dependencies have now been merged in 8.1.beta7

Indeed. I waited for this before ptesting it.

@jdemeyer
Copy link

jdemeyer commented Oct 4, 2017

Reviewer: Frédéric Chapoton, Jeroen Demeyer

@jdemeyer
Copy link

jdemeyer commented Oct 4, 2017

comment:23

One thing which is very annoying is that the build cannot be interrupted with CTRL-C.

@mantepse
Copy link
Contributor Author

mantepse commented Oct 6, 2017

comment:45

As it turns out, also that test fails. When doctesting, after fricas.quit() only some processes are gone immediately.

@jdemeyer
Copy link

jdemeyer commented Oct 7, 2017

comment:46

Replying to @mantepse:

I forwarded your observation to the FriCAS mailing list...

If you have a link to that post, it would be good to put it on this ticket.

@mantepse
Copy link
Contributor Author

mantepse commented Oct 7, 2017

comment:47

I notified upstream, maybe we should remind them before they make the next release:

https://groups.google.com/d/msg/fricas-devel/cmhTjr8IxPU/tY_Idyf2AQAJ

I am currently recompiling sage, to try the suggestion to start fricas with -nosman.

@mantepse
Copy link
Contributor Author

mantepse commented Oct 7, 2017

comment:48

For some reason, using -nosman doesn't work. I can start fricas on the command line with fricas -nosman, and in sage manually, but for some reason it breaks the interface :-(

@mantepse
Copy link
Contributor Author

mantepse commented Oct 7, 2017

comment:49

The issue is a difference in newlines: on linux, with fricas -nosman, I have

sage: fricas.eval("1+1", reformat=False)
'\n|startAlgebraOutput|\n   2\n|endOfAlgebraOutput|'

whereas with fricas -nox -noclef, I have

sage: fricas.eval("1+1", reformat=False)
'\r\n|startAlgebraOutput|\r\n   2\r\n|endOfAlgebraOutput|\r'

I'm not sure what the right fix is. I think I'll replace \r\n throughout with \n.

@mantepse
Copy link
Contributor Author

mantepse commented Oct 7, 2017

Changed commit from e765d16 to f8567eb

@mantepse
Copy link
Contributor Author

mantepse commented Oct 7, 2017

@mantepse
Copy link
Contributor Author

mantepse commented Oct 7, 2017

New commits:

f8567ebset type to optional

@mantepse
Copy link
Contributor Author

mantepse commented Oct 7, 2017

Changed branch from u/mantepse/make_the_experimental_fricas_package_optional to none

@mantepse
Copy link
Contributor Author

mantepse commented Oct 7, 2017

Changed commit from f8567eb to none

@mantepse
Copy link
Contributor Author

mantepse commented Oct 7, 2017

@mantepse
Copy link
Contributor Author

mantepse commented Oct 7, 2017

Commit: e765d16

@mantepse
Copy link
Contributor Author

mantepse commented Oct 7, 2017

comment:53

Apparently, I cannot push anymore. Well, it's working now, I'll try to attach a patch.

@mantepse
Copy link
Contributor Author

mantepse commented Oct 7, 2017

Changed commit from e765d16 to f48e8c5

@mantepse
Copy link
Contributor Author

mantepse commented Oct 7, 2017

@mantepse
Copy link
Contributor Author

mantepse commented Oct 7, 2017

comment:55

should work now! please test on cygwin and MacOS X - I modified the handling of newlines!

@dimpase
Copy link
Member

dimpase commented Oct 8, 2017

comment:56

tested on FreeBSD 11 with clang (thus pretty close to OSX) (cf #22679), all tests in src/sage/interfaces/fricas.py pass.

@mantepse
Copy link
Contributor Author

comment:57

ping?

@dimpase
Copy link
Member

dimpase commented Oct 18, 2017

comment:58

I'll test on OSX later today.

@dimpase
Copy link
Member

dimpase commented Oct 19, 2017

comment:59

OK, all good on OSX.

@dimpase
Copy link
Member

dimpase commented Oct 19, 2017

Changed reviewer from Emmanuel Charpentier, Jeroen Demeyer to Emmanuel Charpentier, Jeroen Demeyer, Dima Pasechnik

@embray
Copy link
Contributor

embray commented Oct 19, 2017

comment:60

LGTM now that that one test is fixed.

@embray
Copy link
Contributor

embray commented Oct 19, 2017

Changed reviewer from Emmanuel Charpentier, Jeroen Demeyer, Dima Pasechnik to Emmanuel Charpentier, Jeroen Demeyer, Dima Pasechnik, Erik Bray

@vbraun
Copy link
Member

vbraun commented Oct 21, 2017

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

6 participants