Move giacpy_sage into sage source code

Question

Move giacpy_sage into sage source code

videlec opened this issue 6 years ago · 69 comments

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

Answer 1 · 2020-02-09T11:05:35.000Z

Branch: public/29171

Answer 2 · 2020-02-09T11:05:35.000Z

New commits:

`8e49ecf`	`2917: move giacpy_sage into Sage source code`

Answer 3 · 2020-02-09T11:05:35.000Z

Commit: 8e49ecf

Answer 4 · 2020-02-09T11:06:10.000Z

Changed commit from 8e49ecf to ac934cb

Answer 5 · 2020-02-09T11:06:10.000Z

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`

Answer 6 · 2020-02-09T23:48:49.000Z

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.

Answer 7 · 2020-02-10T00:06:31.000Z

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

Answer 8 · 2020-04-14T19:41:51.000Z

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.

Answer 9 · 2020-08-10T17:57:56.000Z

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?

Answer 10 · 2020-08-10T19:36:06.000Z

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.

Answer 11 · 2020-08-10T19:51:26.000Z

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

Answer 12 · 2020-08-11T00:28:06.000Z

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`

Answer 13 · 2020-08-11T00:28:06.000Z

Changed commit from ac934cb to d095c37

Answer 14 · 2020-08-11T00:28:38.000Z

comment:14

Rebased on 9.2.beta8

Answer 15 · 2020-08-11T01:12:46.000Z

Dependencies: #30277

Answer 16 · 2020-08-11T01:15:07.000Z

Changed commit from d095c37 to 5d56332

Answer 17 · 2020-08-11T01:15:07.000Z

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`

Answer 18 · 2020-08-11T01:39:18.000Z

Changed author from Vincent Delecroix to Vincent Delecroix, Matthias Koeppe

Answer 19 · 2020-08-11T01:52:05.000Z

Changed commit from 5d56332 to 5471b80

Answer 20 · 2020-08-11T01:52:05.000Z

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`

Answer 21 · 2020-08-11T01:52:23.000Z

Changed dependencies from #30277 to #30277, #29552

Answer 22 · 2020-08-11T01:52:56.000Z

comment:20

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

Answer 23 · 2020-08-11T01:55:56.000Z

Changed commit from 5471b80 to 6a2934e

Answer 24 · 2020-08-11T01:55:56.000Z

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

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

Answer 25 · 2020-08-11T02:33:39.000Z

Changed commit from 6a2934e to aff8292

Answer 26 · 2020-08-11T02:33:39.000Z

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'`

Answer 27 · 2020-08-11T02:38:15.000Z

Changed commit from aff8292 to 1fbe528

Answer 28 · 2020-08-11T02:38:15.000Z

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

`1fbe528`	src/sage/libs/giac/giac.pyx: Update `_latex_` doctest

Answer 29 · 2020-08-11T02:40:36.000Z

comment:25

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

Answer 30 · 2020-08-14T14:31:40.000Z

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

Answer 31 · 2020-08-14T14:31:40.000Z

Changed commit from 1fbe528 to 0ad89d8

Answer 32 · 2020-08-14T14:49:30.000Z

comment:28

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.

Answer 33 · 2020-08-14T15:22:35.000Z

comment:29

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

Answer 34 · 2020-08-14T15:25:12.000Z

comment:30

Replying to @frederichan-IMJPRG:

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?

Answer 35 · 2020-08-14T17:01:18.000Z

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

Answer 36 · 2020-08-14T17:01:18.000Z

Changed commit from 0ad89d8 to 17bc2e4

Answer 37 · 2020-08-14T17:02:11.000Z

comment:32

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.

Answer 38 · 2020-08-14T17:02:26.000Z

Changed dependencies from #30277, #29552 to #30277

Answer 39 · 2020-08-14T17:04:15.000Z

Reviewer: Matthias Koeppe, ...

Answer 40 · 2020-08-14T17:04:15.000Z

Changed author from Vincent Delecroix, Matthias Koeppe to Vincent Delecroix, Matthias Koeppe, Frederic Han

Answer 41 · 2020-08-14T17:14:47.000Z

Changed reviewer from Matthias Koeppe, ... to Matthias Koeppe, github.com/mkoeppe/sage/actions/runs/208891335, github.com/mkoeppe/sage/actions/runs/208891338, github.com/mkoeppe/sage/actions/runs/208891334, ...

Answer 42 · 2020-08-14T17:31:15.000Z

comment:36

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

Answer 43 · 2020-08-15T12:01:37.000Z

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`

Answer 44 · 2020-08-15T12:01:37.000Z

Changed commit from 17bc2e4 to 2f528a1

Answer 45 · 2020-08-15T21:20:43.000Z

comment:38

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

Answer 46 · 2020-08-15T21:21:27.000Z

Changed reviewer from Matthias Koeppe, github.com/mkoeppe/sage/actions/runs/208891335, github.com/mkoeppe/sage/actions/runs/208891338, github.com/mkoeppe/sage/actions/runs/208891334, ... to Matthias Koeppe, ...

Answer 47 · 2020-08-16T07:02:23.000Z

Changed commit from 2f528a1 to 2e44aa6

Answer 48 · 2020-08-16T07:02:23.000Z

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

`2e44aa6`	`remove optional giacpy_sage from multi_polynomial_ideal`

Answer 49 · 2020-08-16T07:36:19.000Z

comment:41

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?

Answer 50 · 2020-08-16T08:48:23.000Z

comment:42

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

Answer 51 · 2020-08-16T12:57:36.000Z

comment:43

Replying to @frederichan-IMJPRG:

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.

Answer 52 · 2020-08-16T13:08:17.000Z

comment:44

Replying to @frederichan-IMJPRG:

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.

Answer 53 · 2020-08-16T13:17:03.000Z

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.

Answer 54 · 2020-08-17T09:11:50.000Z

Changed commit from 2e44aa6 to db48d99

Answer 55 · 2020-08-17T09:11:50.000Z

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

`db48d99`	`add a lazy import for libgiac`

Answer 56 · 2020-08-17T09:38:25.000Z

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

Answer 57 · 2020-08-17T18:09:19.000Z

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?

Answer 58 · 2020-08-17T18:10:22.000Z

comment:49

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

Answer 59 · 2020-08-18T09:44:50.000Z

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

Answer 60 · 2020-08-18T13:32:08.000Z

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

Answer 61 · 2020-08-19T22:17:32.000Z

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

Answer 62 · 2020-08-19T23:32:26.000Z

Changed commit from db48d99 to 911ddfb

Answer 63 · 2020-08-19T23:32:26.000Z

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`

Answer 64 · 2020-08-19T23:33:07.000Z

comment:54

Cherry-picked one commit from #29541

Answer 65 · 2020-08-20T13:38:24.000Z

comment:55

Quick review?

Answer 66 · 2020-08-20T15:08:05.000Z

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

Answer 67 · 2020-08-20T15:08:05.000Z

comment:56

lgtm

Answer 68 · 2020-08-20T18:26:03.000Z

comment:57

Thanks

Answer 69 · 2020-08-23T17:51:11.000Z

Changed branch from public/29171 to 911ddfb