Move giacpy_sage into sage source code

[videlec]

As discussed on this sage-devel thread, we propose to move the Cython code that used to be in the optional package giacpy_sage into sage/libs/giac/.

Depends on #30277

CC: @frederichan-IMJPRG @mwageringel @slel @kiwifb

Component: interfaces

Author: Vincent Delecroix, Matthias Koeppe, Frederic Han

Branch/Commit: 911ddfb

Reviewer: Matthias Koeppe, Frederic Han, Dima Pasechnik

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

[videlec]

Branch: public/29171

[videlec]

New commits:

8e49ecf

2917: move giacpy_sage into Sage source code

[videlec]

Commit: 8e49ecf

[sagetrac-git]

Changed commit from 8e49ecf to ac934cb

[sagetrac-git]

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

ac934cb

29171: move giacpy_sage into Sage source code

[frederichan-IMJPRG]

comment:3

Thank you.
The doc you want for a giac keyword (Ex: gbasis) is avaible like this:

sage: from sage.libs.giac.giac import *
sage: libgiac.gbasis._help()
sage: libgiac.gbasis._sage_doc_()

Indeed
libgiac.gbasis? doesn't give the good help string.
NB: you removed the def help(self):
so now libgiac.gbasis.help() crashes. You should also remove the property help or use _help in it.

[frederichan-IMJPRG]

comment:4

NB: we should think about the new name you chose. Ex backward compatibility or confusions?

Ex: in the past I had often to say: "This already appears in giac, it doesn't come from giacpy_sage".
giac is already the name of the C++ library, the name of the external giac program and the pexpect interface...

[mkoeppe]

comment:5

Batch modifying tickets that will likely not be ready for 9.1, based on a review of the ticket title, branch/review status, and last modification date.

[mkoeppe]

comment:8

Currently giacpy_sage seems to try to support older Sage versions (see https://gitlab.math.univ-paris-diderot.fr/han/giacpy-sage/blob/master/setup.py).

Frederic, is this something important to preserve, or is it fine to merge these files into Sage?

[frederichan-IMJPRG]

comment:10

Replying to @mkoeppe:

Currently giacpy_sage seems to try to support older Sage versions (see https://gitlab.math.univ-paris-diderot.fr/han/giacpy-sage/blob/master/setup.py).

Frederic, is this something important to preserve, or is it fine to merge these files into Sage?

No I think it this situation where giacpy_sage is not a package anymore it is not important to keep backward comptatibility. I think here it is better to just drop the giacpy_sage setup.py and let sage build it via the entry in module.py as vincent did in this branch.

[mkoeppe]

comment:11

Thanks for the quick reaction. Yes, we would just take the Cython files.
Part of my question, I guess, was to make sure that there is no plan to continue development in https://gitlab.math.univ-paris-diderot.fr/han/giacpy-sage/ after we merge the Cython files into Sage. (That would be a fork and increase, not decrease, maintenance burden.)

[sagetrac-git]

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

d095c37

29171: move giacpy_sage into Sage source code

[sagetrac-git]

Changed commit from ac934cb to d095c37

[mkoeppe]

comment:14

Rebased on 9.2.beta8

[mkoeppe]

Dependencies: #30277

[sagetrac-git]

Changed commit from d095c37 to 5d56332

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

0473ef3	Merge branch 't/21559/change-src-bin-installation' into t/29950/build_sagelib_using_installed_sage_setup
7244371	Merge branch 't/29701/replace_use_of_module_list_optionalextension' into t/29950/build_sagelib_using_installed_sage_setup
4344f89	Merge branch 't/21559/change-src-bin-installation' into t/29950/build_sagelib_using_installed_sage_setup
01b96b0	Merge branch 't/29701/replace_use_of_module_list_optionalextension' into t/29950/build_sagelib_using_installed_sage_setup
2818739	Merge branch 't/29950/build_sagelib_using_installed_sage_setup' into t/30277/remove_src_module_list_py
8a19fe2	build/make/Makefile.in (sagelib-clean): Clean the new build location
ccc67b0	src/sage_setup: Update cythonized_dir in doctests
df38027	Merge branch 't/29950/build_sagelib_using_installed_sage_setup' into t/30277/remove_src_module_list_py
6bc3f60	Merge branch 't/30277/remove_src_module_list_py' into t/29171/public/29171
5d56332	sage/libs/giac/giac.pyx: Add distutils directives

[mkoeppe]

Changed author from Vincent Delecroix to Vincent Delecroix, Matthias Koeppe

[sagetrac-git]

Changed commit from 5d56332 to 5471b80

[mkoeppe]

comment:45

Replying to @frederichan-IMJPRG:

also how should we deal with backward compatibility?
Example: in snappy tree i see some: from giacpy_sage import libgiac here
https://bitbucket.org/mgoerner/snappy/src/default/dev/extended_ptolemy/giac_helper.py

I would suggest that we don't do anything, leaving it to the user libraries to add some try/except to their imports.

[sagetrac-git]

Changed commit from 2e44aa6 to db48d99

[sagetrac-git]

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

db48d99

add a lazy import for libgiac

[frederichan-IMJPRG]

comment:47

Ok so I have added giacpy-mkeywords.py in src/sage_setup/autogen and added a lazy import.
about the message
Help file /Applications/usr/share/giac/doc/en/aide_cas not found
I don't think that it is a broken installation. This text file have a builtin version in the library. I think it is only usefull if you want to overide the builtin version without recompiling giac.
Ex: if in icas I do ?smith I have the same information as in share/giac/doc/aide_cas

[mkoeppe]

comment:48

Replying to @frederichan-IMJPRG:

Ok so I have added giacpy-mkeywords.py in src/sage_setup/autogen and added a lazy import.

Looks great.

about the message
Help file /Applications/usr/share/giac/doc/en/aide_cas not found
I don't think that it is a broken installation. This text file have a builtin version in the library. I think it is only usefull if you want to overide the builtin version without recompiling giac.
Ex: if in icas I do ?smith I have the same information as in share/giac/doc/aide_cas

Thanks for the information. It's just that it looks like an error message, and moreover the path (/Applications/usr...) is really strange and is unrelated to the actual installation location of giac in sage.

Perhaps upstream could add a way to suppress these messages?

[mkoeppe]

comment:49

Ready for "positive review" from my side...

[frederichan-IMJPRG]

comment:50

So do I. (I know that there is room for improvement in the speed of evaluation of functions as vincent suggested in its first branch but it would not be a minor change. I'd rather in a first step keep things close to the giacpy_sage package to see if there are problems in the migration, and improve later with a list of functions that wants a direct implementation for speed.)

[mkoeppe]

Changed reviewer from Matthias Koeppe, ... to Matthias Koeppe, Frederic Han

[vbraun]

comment:52

sage -t --long --warn-long 60.7 --random-seed=0 src/sage/interfaces/giac.py
**********************************************************************
File "src/sage/interfaces/giac.py", line 1002, in sage.interfaces.giac.GiacElement._latex_
Failed example:
    latex(gM)
Expected:
    \left(\begin{array}{cc}
    1 & 2 \\
    3 & 4
    \end{array}\right)
Got:
    \left[\begin{array}{cc}1&2\\3&4\end{array}\right]
**********************************************************************
1 item had failures:
   1 of   7 in sage.interfaces.giac.GiacElement._latex_
    [172 tests, 1 failure, 2.47 s]
----------------------------------------------------------------------

[sagetrac-git]

Changed commit from db48d99 to 911ddfb

[sagetrac-git]

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

923f8f3	Merge tag '9.2.beta9' into t/29171/public/29171
911ddfb	GiacElement._latex_: Make array doctest more flexible

[mkoeppe]

comment:54

Cherry-picked one commit from #29541

[mkoeppe]

comment:55

Quick review?

[dimpase]

Changed reviewer from Matthias Koeppe, Frederic Han to Matthias Koeppe, Frederic Han, Dima Pasechnik

[dimpase]

comment:56

lgtm

[mkoeppe]

comment:57

Thanks

[vbraun]

Changed branch from public/29171 to 911ddfb