Opened 3 years ago

Closed 2 years ago

#29171 closed enhancement (fixed)

Move giacpy_sage into sage source code

Reported by: Vincent Delecroix Owned by:
Priority: critical Milestone: sage-9.2
Component: interfaces Keywords:
Cc: Han Frederic, Markus Wageringel, Samuel Lelièvre, François Bissey Merged in:
Authors: Vincent Delecroix, Matthias Koeppe, Frederic Han Reviewers: Matthias Koeppe, Frederic Han, Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 911ddfb (Commits, GitHub, GitLab) Commit: 911ddfbe2fd809473e82eaf0bac1a4c50858d161
Dependencies: #30277 Stopgaps:

Status badges

Description

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

Change History (58)

comment:1 Changed 3 years ago by Vincent Delecroix

Branch: public/29171
Commit: 8e49ecf0d50dbc20bc9531bc41d811f8c830ac86

New commits:

8e49ecf2917: move giacpy_sage into Sage source code

comment:2 Changed 3 years ago by git

Commit: 8e49ecf0d50dbc20bc9531bc41d811f8c830ac86ac934cbc07f70f8a7d9dbeee7563b57d323cfb8f

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

ac934cb29171: move giacpy_sage into Sage source code

comment:3 Changed 3 years ago by 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.

comment:4 Changed 3 years ago by 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...

comment:5 Changed 3 years ago by Matthias Köppe

Milestone: sage-9.1sage-9.2

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.

comment:6 Changed 2 years ago by Matthias Köppe

Priority: majorcritical

comment:7 Changed 2 years ago by Matthias Köppe

Cc: Han Frederic Markus Wageringel Samuel Lelièvre added

comment:8 Changed 2 years ago by 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?

comment:9 Changed 2 years ago by Matthias Köppe

Status: newneeds_info

comment:10 in reply to:  8 Changed 2 years ago by 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.

comment:11 Changed 2 years ago by 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.)

comment:12 Changed 2 years ago by Matthias Köppe

Cc: François Bissey added

comment:13 Changed 2 years ago by git

Commit: ac934cbc07f70f8a7d9dbeee7563b57d323cfb8fd095c37d862707b57721fb9c8d85b4b39b7c3b51

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

d095c3729171: move giacpy_sage into Sage source code

comment:14 Changed 2 years ago by Matthias Köppe

Rebased on 9.2.beta8

comment:15 Changed 2 years ago by Matthias Köppe

Dependencies: #30277

comment:16 Changed 2 years ago by git

Commit: d095c37d862707b57721fb9c8d85b4b39b7c3b515d56332fafce16b294eef025855f8af9edce151a

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

0473ef3Merge branch 't/21559/change-src-bin-installation' into t/29950/build_sagelib_using_installed_sage_setup
7244371Merge branch 't/29701/replace_use_of_module_list_optionalextension' into t/29950/build_sagelib_using_installed_sage_setup
4344f89Merge branch 't/21559/change-src-bin-installation' into t/29950/build_sagelib_using_installed_sage_setup
01b96b0Merge branch 't/29701/replace_use_of_module_list_optionalextension' into t/29950/build_sagelib_using_installed_sage_setup
2818739Merge branch 't/29950/build_sagelib_using_installed_sage_setup' into t/30277/remove_src_module_list_py
8a19fe2build/make/Makefile.in (sagelib-clean): Clean the new build location
ccc67b0src/sage_setup: Update cythonized_dir in doctests
df38027Merge branch 't/29950/build_sagelib_using_installed_sage_setup' into t/30277/remove_src_module_list_py
6bc3f60Merge branch 't/30277/remove_src_module_list_py' into t/29171/public/29171
5d56332sage/libs/giac/giac.pyx: Add distutils directives

comment:17 Changed 2 years ago by Matthias Köppe

Authors: Vincent DelecroixVincent Delecroix, Matthias Koeppe
Status: needs_infoneeds_review

comment:18 Changed 2 years ago by git

Commit: 5d56332fafce16b294eef025855f8af9edce151a5471b803be3861e8c2e391c9625647ec49bfa8e5

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

5239d30build/pkgs/giac: Update to 1.5.0.87
8bbd056sage.interfaces.giac.GiacElement._latex_: Fix implementation and doctest, make doctest more flexible
5471b80Merge branch 't/29552/upgrade_giac_to_1_5_0_87' into HEAD

comment:19 Changed 2 years ago by Matthias Köppe

Dependencies: #30277#30277, #29552

comment:20 Changed 2 years ago by 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]                                ^

comment:21 Changed 2 years ago by git

Commit: 5471b803be3861e8c2e391c9625647ec49bfa8e56a2934ec8258214cfe2708ce935c01b1f9b99b80

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

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

comment:22 Changed 2 years ago by Matthias Köppe

Status: needs_reviewneeds_work

comment:23 Changed 2 years ago by git

Commit: 6a2934ec8258214cfe2708ce935c01b1f9b99b80aff8292282fcf9bab697f862c66ce5988893ed1e

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

a3793c8Rename src/sage/libs/giac.py to src/sage/libs/giac/__init__.py
5445b2fsage.libs.giac: Update import statements
aff8292Remove '# optional - giacpy_sage'

comment:24 Changed 2 years ago by git

Commit: aff8292282fcf9bab697f862c66ce5988893ed1e1fbe52816c7293bc2e3680d55c389609e5898438

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

1fbe528src/sage/libs/giac/giac.pyx: Update _latex_ doctest

comment:25 Changed 2 years ago by 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
**********************************************************************

comment:26 Changed 2 years ago by Matthias Köppe

Milestone: sage-9.2sage-9.3

comment:27 Changed 2 years ago by git

Commit: 1fbe52816c7293bc2e3680d55c389609e58984380ad89d8ede5b4e6587c0cf95ab92c17b964caf18

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

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

comment:28 Changed 2 years ago by 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.

comment:29 Changed 2 years ago by 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

Last edited 2 years ago by Matthias Köppe (previous) (diff)

comment:30 in reply to:  4 Changed 2 years ago by 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?

comment:31 Changed 2 years ago by git

Commit: 0ad89d8ede5b4e6587c0cf95ab92c17b964caf1817bc2e47342a58de0c73d7593df07c333e2f7dd4

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

f90e37dsrc/sage/libs/giac/misc.h: Add missing C++ include
fce9cf8Rename src/sage/libs/giac.py to src/sage/libs/giac/__init__.py
7c80909sage.libs.giac: Update import statements
f34aef0Remove '# optional - giacpy_sage'
0338556src/sage/libs/giac/giac.pyx: Update _latex_ doctest
798f6c2doctest fixes + __len__ modification for giac 1.5.0.87 behavior with emptylist seq[]
17bc2e4sage/libs/giac/giac.pyx: Make _latex_ doctest more flexible

comment:32 in reply to:  29 Changed 2 years ago by Matthias Köppe

Milestone: sage-9.3sage-9.2
Status: needs_workneeds_review

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.

comment:33 Changed 2 years ago by Matthias Köppe

Dependencies: #30277, #29552#30277

comment:34 Changed 2 years ago by Matthias Köppe

Authors: Vincent Delecroix, Matthias KoeppeVincent Delecroix, Matthias Koeppe, Frederic Han
Reviewers: Matthias Koeppe, ...

comment:35 Changed 2 years ago by Matthias Köppe

Reviewers: Matthias Koeppe, ...Matthias Koeppe, github.com/mkoeppe/sage/actions/runs/208891335, github.com/mkoeppe/sage/actions/runs/208891338, github.com/mkoeppe/sage/actions/runs/208891334, ...

comment:36 Changed 2 years ago by Matthias Köppe

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

comment:37 Changed 2 years ago by git

Commit: 17bc2e47342a58de0c73d7593df07c333e2f7dd42f528a1d31dfb81a7c229cf376a5fcfe9ee677ba

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

8bbd056sage.interfaces.giac.GiacElement._latex_: Fix implementation and doctest, make doctest more flexible
5471b80Merge branch 't/29552/upgrade_giac_to_1_5_0_87' into HEAD
6a2934esrc/sage/libs/giac/misc.h: Add missing C++ include
a3793c8Rename src/sage/libs/giac.py to src/sage/libs/giac/__init__.py
5445b2fsage.libs.giac: Update import statements
aff8292Remove '# optional - giacpy_sage'
1fbe528src/sage/libs/giac/giac.pyx: Update _latex_ doctest
0ad89d8doctest fixes + __len__ modification for giac 1.5.0.87 behavior with emptylist seq[]
7d25a4bMerge branch 'public/29171' of git://trac.sagemath.org/sage into public/29171
2f528a1put giac methods in a separate file with doctrings from giac's help

comment:38 Changed 2 years ago by Matthias Köppe

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

comment:39 Changed 2 years ago by Matthias Köppe

Reviewers: Matthias Koeppe, github.com/mkoeppe/sage/actions/runs/208891335, github.com/mkoeppe/sage/actions/runs/208891338, github.com/mkoeppe/sage/actions/runs/208891334, ...Matthias Koeppe, ...

comment:40 Changed 2 years ago by git

Commit: 2f528a1d31dfb81a7c229cf376a5fcfe9ee677ba2e44aa6a3ea5e4c2dfc281ce82f2a52a90ca60ce

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

2e44aa6remove optional giacpy_sage from multi_polynomial_ideal

comment:41 Changed 2 years ago by 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?

comment:42 Changed 2 years ago by 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

comment:43 in reply to:  42 Changed 2 years ago by 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.

comment:44 in reply to:  41 Changed 2 years ago by 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.

comment:45 in reply to:  42 Changed 2 years ago by 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.

comment:46 Changed 2 years ago by git

Commit: 2e44aa6a3ea5e4c2dfc281ce82f2a52a90ca60cedb48d99b8f5f85aa7794e7447feec89ae267d2e6

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

db48d99add a lazy import for libgiac

comment:47 Changed 2 years ago by 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

comment:48 in reply to:  47 Changed 2 years ago by 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?

comment:49 Changed 2 years ago by Matthias Köppe

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

comment:50 Changed 2 years ago by 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.)

comment:51 Changed 2 years ago by Matthias Köppe

Reviewers: Matthias Koeppe, ...Matthias Koeppe, Frederic Han
Status: needs_reviewpositive_review

comment:52 Changed 2 years ago by Volker Braun

Status: positive_reviewneeds_work
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]
----------------------------------------------------------------------

comment:53 Changed 2 years ago by git

Commit: db48d99b8f5f85aa7794e7447feec89ae267d2e6911ddfbe2fd809473e82eaf0bac1a4c50858d161

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

923f8f3Merge tag '9.2.beta9' into t/29171/public/29171
911ddfbGiacElement._latex_: Make array doctest more flexible

comment:54 Changed 2 years ago by Matthias Köppe

Status: needs_workneeds_review

Cherry-picked one commit from #29541

comment:55 Changed 2 years ago by Matthias Köppe

Quick review?

comment:56 Changed 2 years ago by Dima Pasechnik

Reviewers: Matthias Koeppe, Frederic HanMatthias Koeppe, Frederic Han, Dima Pasechnik
Status: needs_reviewpositive_review

lgtm

comment:57 Changed 2 years ago by Matthias Köppe

Thanks

comment:58 Changed 2 years ago by Volker Braun

Branch: public/29171911ddfbe2fd809473e82eaf0bac1a4c50858d161
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.