Opened 8 months ago

Closed 5 weeks ago

#29171 closed enhancement (fixed)

Move giacpy_sage into sage source code

Reported by: vdelecroix Owned by:
Priority: critical Milestone: sage-9.2
Component: interfaces Keywords:
Cc: frederichan, gh-mwageringel, slelievre, fbissey 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) Commit: 911ddfbe2fd809473e82eaf0bac1a4c50858d161
Dependencies: #30277 Stopgaps:

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 8 months ago by vdelecroix

  • Branch set to public/29171
  • Commit set to 8e49ecf0d50dbc20bc9531bc41d811f8c830ac86

New commits:

8e49ecf2917: move giacpy_sage into Sage source code

comment:2 Changed 8 months ago by git

  • Commit changed from 8e49ecf0d50dbc20bc9531bc41d811f8c830ac86 to ac934cbc07f70f8a7d9dbeee7563b57d323cfb8f

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 8 months ago by frederichan

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 follow-up: Changed 8 months ago by 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...

comment:5 Changed 5 months ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-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 7 weeks ago by mkoeppe

  • Priority changed from major to critical

comment:7 Changed 7 weeks ago by mkoeppe

  • Cc frederichan gh-mwageringel slelievre added

comment:8 follow-up: Changed 7 weeks ago by 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?

comment:9 Changed 7 weeks ago by mkoeppe

  • Status changed from new to needs_info

comment:10 in reply to: ↑ 8 Changed 7 weeks ago by frederichan

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 7 weeks ago by mkoeppe

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 7 weeks ago by mkoeppe

  • Cc fbissey added

comment:13 Changed 7 weeks ago by git

  • Commit changed from ac934cbc07f70f8a7d9dbeee7563b57d323cfb8f to d095c37d862707b57721fb9c8d85b4b39b7c3b51

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 7 weeks ago by mkoeppe

Rebased on 9.2.beta8

comment:15 Changed 7 weeks ago by mkoeppe

  • Dependencies set to #30277

comment:16 Changed 7 weeks ago by git

  • Commit changed from d095c37d862707b57721fb9c8d85b4b39b7c3b51 to 5d56332fafce16b294eef025855f8af9edce151a

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 7 weeks ago by mkoeppe

  • Authors changed from Vincent Delecroix to Vincent Delecroix, Matthias Koeppe
  • Status changed from needs_info to needs_review

comment:18 Changed 7 weeks ago by git

  • Commit changed from 5d56332fafce16b294eef025855f8af9edce151a to 5471b803be3861e8c2e391c9625647ec49bfa8e5

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 7 weeks ago by mkoeppe

  • Dependencies changed from #30277 to #30277, #29552

comment:20 Changed 7 weeks ago by mkoeppe

[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 7 weeks ago by git

  • Commit changed from 5471b803be3861e8c2e391c9625647ec49bfa8e5 to 6a2934ec8258214cfe2708ce935c01b1f9b99b80

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

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

comment:22 Changed 7 weeks ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:23 Changed 7 weeks ago by git

  • Commit changed from 6a2934ec8258214cfe2708ce935c01b1f9b99b80 to aff8292282fcf9bab697f862c66ce5988893ed1e

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 7 weeks ago by git

  • Commit changed from aff8292282fcf9bab697f862c66ce5988893ed1e to 1fbe52816c7293bc2e3680d55c389609e5898438

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

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

comment:25 Changed 7 weeks ago by mkoeppe

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 6 weeks ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:27 Changed 6 weeks ago by git

  • Commit changed from 1fbe52816c7293bc2e3680d55c389609e5898438 to 0ad89d8ede5b4e6587c0cf95ab92c17b964caf18

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 6 weeks ago by frederichan

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 follow-up: Changed 6 weeks ago by 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

Last edited 6 weeks ago by mkoeppe (previous) (diff)

comment:30 in reply to: ↑ 4 Changed 6 weeks ago by mkoeppe

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 6 weeks ago by git

  • Commit changed from 0ad89d8ede5b4e6587c0cf95ab92c17b964caf18 to 17bc2e47342a58de0c73d7593df07c333e2f7dd4

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 6 weeks ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.2
  • Status changed from needs_work to needs_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 6 weeks ago by mkoeppe

  • Dependencies changed from #30277, #29552 to #30277

comment:34 Changed 6 weeks ago by mkoeppe

  • Authors changed from Vincent Delecroix, Matthias Koeppe to Vincent Delecroix, Matthias Koeppe, Frederic Han
  • Reviewers set to Matthias Koeppe, ...

comment:35 Changed 6 weeks ago by mkoeppe

  • Reviewers changed 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, ...

comment:36 Changed 6 weeks ago by mkoeppe

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

comment:37 Changed 6 weeks ago by git

  • Commit changed from 17bc2e47342a58de0c73d7593df07c333e2f7dd4 to 2f528a1d31dfb81a7c229cf376a5fcfe9ee677ba

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 6 weeks ago by mkoeppe

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

comment:39 Changed 6 weeks ago by mkoeppe

  • Reviewers changed 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, ...

comment:40 Changed 6 weeks ago by git

  • Commit changed from 2f528a1d31dfb81a7c229cf376a5fcfe9ee677ba to 2e44aa6a3ea5e4c2dfc281ce82f2a52a90ca60ce

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

2e44aa6remove optional giacpy_sage from multi_polynomial_ideal

comment:41 follow-up: Changed 6 weeks ago by frederichan

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 follow-ups: Changed 6 weeks ago by 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)

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 6 weeks ago by mkoeppe

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 6 weeks ago by mkoeppe

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 6 weeks ago by mkoeppe

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 6 weeks ago by git

  • Commit changed from 2e44aa6a3ea5e4c2dfc281ce82f2a52a90ca60ce to db48d99b8f5f85aa7794e7447feec89ae267d2e6

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

db48d99add a lazy import for libgiac

comment:47 follow-up: Changed 6 weeks ago by frederichan

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 6 weeks ago by mkoeppe

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 6 weeks ago by mkoeppe

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

comment:50 Changed 6 weeks ago by frederichan

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 5 weeks ago by mkoeppe

  • Reviewers changed from Matthias Koeppe, ... to Matthias Koeppe, Frederic Han
  • Status changed from needs_review to positive_review

comment:52 Changed 5 weeks ago by vbraun

  • Status changed from positive_review to needs_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 5 weeks ago by git

  • Commit changed from db48d99b8f5f85aa7794e7447feec89ae267d2e6 to 911ddfbe2fd809473e82eaf0bac1a4c50858d161

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 5 weeks ago by mkoeppe

  • Status changed from needs_work to needs_review

Cherry-picked one commit from #29541

comment:55 Changed 5 weeks ago by mkoeppe

Quick review?

comment:56 Changed 5 weeks ago by dimpase

  • Reviewers changed from Matthias Koeppe, Frederic Han to Matthias Koeppe, Frederic Han, Dima Pasechnik
  • Status changed from needs_review to positive_review

lgtm

comment:57 Changed 5 weeks ago by mkoeppe

Thanks

comment:58 Changed 5 weeks ago by vbraun

  • Branch changed from public/29171 to 911ddfbe2fd809473e82eaf0bac1a4c50858d161
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.