Move giacpy_sage into sage source code

[Vincent Delecroix]

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/.

[Vincent Delecroix]

New commits:

​8e49ecf

2917: move giacpy_sage into Sage source code

[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

[Han Frederic]

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.

[Han Frederic]

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...

[Matthias Köppe]

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.

[Matthias Köppe]

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?

[Han Frederic]

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.

[Matthias Köppe]

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

[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

[Matthias Köppe]

Rebased on 9.2.beta8

[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

[git]

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

​5239d30	build/pkgs/giac: Update to 1.5.0.87
​8bbd056	sage.interfaces.giac.GiacElement._latex_: Fix implementation and doctest, make doctest more flexible
​5471b80	Merge branch 't/29552/upgrade_giac_to_1_5_0_87' into HEAD

[Matthias Köppe]

[sagelib-9.2.beta8] In file included from build/cythonized/sage/libs/giac/giac.cpp:7695:
[sagelib-9.2.beta8] build/cythonized/sage/libs/giac/misc.h:104:12: error: implicit instantiation of undefined template 'std::__1::basic_ofstream<char, std::__1::char_traits<char> >'
[sagelib-9.2.beta8]   ofstream of(filename.c_str());
[sagelib-9.2.beta8]            ^
[sagelib-9.2.beta8] /Library/Developer/CommandLineTools/usr/bin/../include/c++/v1/iosfwd:146:32: note: template is declared here
[sagelib-9.2.beta8]     class _LIBCPP_TEMPLATE_VIS basic_ofstream;
[sagelib-9.2.beta8]                                ^
[sagelib-9.2.beta8] In file included from build/cythonized/sage/libs/giac/giac.cpp:7695:
[sagelib-9.2.beta8] build/cythonized/sage/libs/giac/misc.h:110:12: error: implicit instantiation of undefined template 'std::__1::basic_ifstream<char, std::__1::char_traits<char> >'
[sagelib-9.2.beta8]   ifstream f(filename.c_str());
[sagelib-9.2.beta8]            ^
[sagelib-9.2.beta8] /Library/Developer/CommandLineTools/usr/bin/../include/c++/v1/iosfwd:144:32: note: template is declared here
[sagelib-9.2.beta8]     class _LIBCPP_TEMPLATE_VIS basic_ifstream;
[sagelib-9.2.beta8]                                ^

[git]

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

​6a2934e

src/sage/libs/giac/misc.h: Add missing C++ include

[git]

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

​a3793c8	Rename src/sage/libs/giac.py to src/sage/libs/giac/__init__.py
​5445b2f	sage.libs.giac: Update import statements
​aff8292	Remove '# optional - giacpy_sage'

[git]

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

​1fbe528

src/sage/libs/giac/giac.pyx: Update _latex_ doctest

[Matthias Köppe]

Several errors remain -- expert help is needed

sage -t --random-seed=0 src/sage/libs/giac/giac.pyx
**********************************************************************
File "src/sage/libs/giac/giac.pyx", line 30, in sage.libs.giac.giac
Failed example:
    from sage.libs.giac.giac import *
Expected nothing
Got:
    Help file /Applications/usr/share/giac/doc/fr/aide_cas not found
    Added 0 synonyms
**********************************************************************
File "src/sage/libs/giac/giac.pyx", line 980, in sage.libs.giac.giac.encstring23.GiacSetting.Pygen.__getitem__
Failed example:
    l[0]
Expected:
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    ...
    IndexError: list index 0 out of range
Got:
    <BLANKLINE>
    Traceback (most recent call last):
      File "sage/libs/giac/giac.pyx", line 996, in sage.libs.giac.giac.Pygen.__getitem__ (build/cythonized/sage/libs/giac/giac.cpp:12218)
        result=self.gptr[0][<int>i]
    RuntimeError: Index outside range : 0, vector size is 0, syntax compatibility mode xcas
     Error: Invalid dimension
    <BLANKLINE>
    During handling of the above exception, another exception occurred:
    <BLANKLINE>
    Traceback (most recent call last):
      File "/Users/mkoeppe/s/sage/sage-rebasing/local/lib/python3.7/site-packages/sage/doctest/forker.py", line 715, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/Users/mkoeppe/s/sage/sage-rebasing/local/lib/python3.7/site-packages/sage/doctest/forker.py", line 1139, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.libs.giac.giac.encstring23.GiacSetting.Pygen.__getitem__[11]>", line 1, in <module>
        l[Integer(0)]
      File "sage/libs/giac/giac.pyx", line 1000, in sage.libs.giac.giac.Pygen.__getitem__ (build/cythonized/sage/libs/giac/giac.cpp:12285)
        raise RuntimeError
    RuntimeError
**********************************************************************
File "src/sage/libs/giac/giac.pyx", line 985, in sage.libs.giac.giac.encstring23.GiacSetting.Pygen.__getitem__
Failed example:
    sig_on_count() # check sig_on/off pairings (virtual doctest)
Expected:
    0
Got:
    1
**********************************************************************

[git]

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

​0ad89d8

doctest fixes + __len__ modification for giac 1.5.0.87 behavior with emptylist seq[]

[Han Frederic]

NB: the first time that giacpy is loaded the C++ library outputs an OS dependant message so we need to put a # random flag in the doctests at the first import

I have modified giacpy len to work with 1.5.0.87 whatever if we patch it or not.

[Matthias Köppe]

Does this also work with earlier 1.5.0.x? Then perhaps we can go forward with this ticket without the upgrade in #29552

[Matthias Köppe]

Replying to frederichan:

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...

Sorry I missed this comment. Are you referring to the name of the module, sage.libs.giac?

[git]

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

​f90e37d	src/sage/libs/giac/misc.h: Add missing C++ include
​fce9cf8	Rename src/sage/libs/giac.py to src/sage/libs/giac/__init__.py
​7c80909	sage.libs.giac: Update import statements
​f34aef0	Remove '# optional - giacpy_sage'
​0338556	src/sage/libs/giac/giac.pyx: Update _latex_ doctest
​798f6c2	doctest fixes + __len__ modification for giac 1.5.0.87 behavior with emptylist seq[]
​17bc2e4	sage/libs/giac/giac.pyx: Make _latex_ doctest more flexible

[Matthias Köppe]

Replying to mkoeppe:

Does this also work with earlier 1.5.0.x? Then perhaps we can go forward with this ticket without the upgrade in #29552

OK, I have rebased it, dropping the merge of the upgrade ticket #29552.

This seems to work.

[Matthias Köppe]

It would probably be good to add some docstrings, for example for GiacInstance...

[git]

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

​8bbd056	sage.interfaces.giac.GiacElement._latex_: Fix implementation and doctest, make doctest more flexible
​5471b80	Merge branch 't/29552/upgrade_giac_to_1_5_0_87' into HEAD
​6a2934e	src/sage/libs/giac/misc.h: Add missing C++ include
​a3793c8	Rename src/sage/libs/giac.py to src/sage/libs/giac/__init__.py
​5445b2f	sage.libs.giac: Update import statements
​aff8292	Remove '# optional - giacpy_sage'
​1fbe528	src/sage/libs/giac/giac.pyx: Update _latex_ doctest
​0ad89d8	doctest fixes + __len__ modification for giac 1.5.0.87 behavior with emptylist seq[]
​7d25a4b	Merge branch 'public/29171' of git://trac.sagemath.org/sage into public/29171
​2f528a1	put giac methods in a separate file with doctrings from giac's help

[Matthias Köppe]

This seems to work well. We need another reviewer on this ticket...

[git]

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

​2e44aa6

remove optional giacpy_sage from multi_polynomial_ideal

[Han Frederic]

Can we import the libgiac function from sage/libs/all.py such that it is avaiable in a similar way as libgap? the problem if I add lazy_import('sage.libs.giac.giac', 'libgiac') is that the first time one type libgiac(2) the user might be confused by the loading message of the giac library:

sage: libgiac(2)                                                                                              
// Giac share root-directory:/home/fred-dev/sage/develop/sage.new/local/share/giac/
// Giac share root-directory:/home/fred-dev/sage/develop/sage.new/local/share/giac/
Help file /home/fred-dev/sage/develop/sage.new/local/share/giac/doc/fr/aide_cas not found
Added 0 synonyms
2

is it better to not use lazy_import or to force the loading at startup or just keep this message during the first use of libgiac?

[Han Frederic]

I have used this file ​https://gitlab.math.univ-paris-diderot.fr/han/giacpy-sage/blob/master/mkkeywords.py to create the auto-methods.pxi file. But I don't know where to put it. It can be usefull for maintenance but it is not used by giacpy. Building auto-methods.pxi is quite long and too dangerous to be rebuilt on the fly. (not implementing a new giac keyword doesn"t break the giacpy built but some new giac keywords often cause trouble)

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

[Matthias Köppe]

Replying to frederichan:

I have used this file ​https://gitlab.math.univ-paris-diderot.fr/han/giacpy-sage/blob/master/mkkeywords.py to create the auto-methods.pxi file. But I don't know where to put it. It can be usefull for maintenance but it is not used by giacpy. Building auto-methods.pxi is quite long and too dangerous to be rebuilt on the fly. (not implementing a new giac keyword doesn"t break the giacpy built but some new giac keywords often cause trouble)

I think src/sage_setup/autogen/ is the right place for that. Include comments like the above that explain why it's not called automatically.

[Matthias Köppe]

Replying to frederichan:

Can we import the libgiac function from sage/libs/all.py such that it is avaiable in a similar way as libgap?

Yes, this is a good idea - it would match libgap and maxima_calculus, which are available too.

the problem if I add lazy_import('sage.libs.giac.giac', 'libgiac') is that the first time one type libgiac(2) the user might be confused by the loading message of the giac library [...]:

That's not a big problem.

sage: libgiac(2) Giac share root-directory:/home/fred-dev/sage/develop/sage.new/local/share/giac/ Giac share root-directory:/home/fred-dev/sage/develop/sage.new/local/share/giac/

Perhaps open an issue with upstream to reduce verbosity in library mode?

Help file /home/fred-dev/sage/develop/sage.new/local/share/giac/doc/fr/aide_cas not found

There's something wrong with our installation, as mentioned in #29552

is it better to not use lazy_import or to force the loading at startup

No, definitely not. We don't want to load it at startup.

[Matthias Köppe]

Replying to frederichan:

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.

[git]

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

​db48d99

add a lazy import for libgiac

[Han Frederic]

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

[Matthias Köppe]

Replying to frederichan:

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?

[Matthias Köppe]

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

[Han Frederic]

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

[Volker Braun]

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]
----------------------------------------------------------------------

[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

[Matthias Köppe]

Cherry-picked one commit from #29541

[Matthias Köppe]

Quick review?

[Dima Pasechnik]

lgtm

[Matthias Köppe]

Thanks