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:  sage9.2 
Component:  interfaces  Keywords:  
Cc:  frederichan, ghmwageringel, 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 sagedevel 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
 Branch set to public/29171
 Commit set to 8e49ecf0d50dbc20bc9531bc41d811f8c830ac86
comment:2 Changed 8 months ago by
 Commit changed from 8e49ecf0d50dbc20bc9531bc41d811f8c830ac86 to ac934cbc07f70f8a7d9dbeee7563b57d323cfb8f
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

comment:3 Changed 8 months ago by
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 followup: ↓ 30 Changed 8 months ago by
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
 Milestone changed from sage9.1 to sage9.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
 Priority changed from major to critical
comment:7 Changed 7 weeks ago by
 Cc frederichan ghmwageringel slelievre added
comment:8 followup: ↓ 10 Changed 7 weeks ago by
Currently giacpy_sage
seems to try to support older Sage versions (see https://gitlab.math.univparisdiderot.fr/han/giacpysage/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
 Status changed from new to needs_info
comment:10 in reply to: ↑ 8 Changed 7 weeks ago by
Replying to mkoeppe:
Currently
giacpy_sage
seems to try to support older Sage versions (see https://gitlab.math.univparisdiderot.fr/han/giacpysage/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
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.univparisdiderot.fr/han/giacpysage/ 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
 Cc fbissey added
comment:13 Changed 7 weeks ago by
 Commit changed from ac934cbc07f70f8a7d9dbeee7563b57d323cfb8f to d095c37d862707b57721fb9c8d85b4b39b7c3b51
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

comment:14 Changed 7 weeks ago by
Rebased on 9.2.beta8
comment:15 Changed 7 weeks ago by
 Dependencies set to #30277
comment:16 Changed 7 weeks ago by
 Commit changed from d095c37d862707b57721fb9c8d85b4b39b7c3b51 to 5d56332fafce16b294eef025855f8af9edce151a
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
0473ef3  Merge branch 't/21559/changesrcbininstallation' 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/changesrcbininstallation' 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 (sagelibclean): 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

comment:17 Changed 7 weeks ago by
 Status changed from needs_info to needs_review
comment:18 Changed 7 weeks ago by
 Commit changed from 5d56332fafce16b294eef025855f8af9edce151a to 5471b803be3861e8c2e391c9625647ec49bfa8e5
comment:19 Changed 7 weeks ago by
 Dependencies changed from #30277 to #30277, #29552
comment:20 Changed 7 weeks ago by
[sagelib9.2.beta8] In file included from build/cythonized/sage/libs/giac/giac.cpp:7695: [sagelib9.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> >' [sagelib9.2.beta8] ofstream of(filename.c_str()); [sagelib9.2.beta8] ^ [sagelib9.2.beta8] /Library/Developer/CommandLineTools/usr/bin/../include/c++/v1/iosfwd:146:32: note: template is declared here [sagelib9.2.beta8] class _LIBCPP_TEMPLATE_VIS basic_ofstream; [sagelib9.2.beta8] ^ [sagelib9.2.beta8] In file included from build/cythonized/sage/libs/giac/giac.cpp:7695: [sagelib9.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> >' [sagelib9.2.beta8] ifstream f(filename.c_str()); [sagelib9.2.beta8] ^ [sagelib9.2.beta8] /Library/Developer/CommandLineTools/usr/bin/../include/c++/v1/iosfwd:144:32: note: template is declared here [sagelib9.2.beta8] class _LIBCPP_TEMPLATE_VIS basic_ifstream; [sagelib9.2.beta8] ^
comment:21 Changed 7 weeks ago by
 Commit changed from 5471b803be3861e8c2e391c9625647ec49bfa8e5 to 6a2934ec8258214cfe2708ce935c01b1f9b99b80
Branch pushed to git repo; I updated commit sha1. New commits:
6a2934e  src/sage/libs/giac/misc.h: Add missing C++ include

comment:22 Changed 7 weeks ago by
 Status changed from needs_review to needs_work
comment:23 Changed 7 weeks ago by
 Commit changed from 6a2934ec8258214cfe2708ce935c01b1f9b99b80 to aff8292282fcf9bab697f862c66ce5988893ed1e
comment:24 Changed 7 weeks ago by
 Commit changed from aff8292282fcf9bab697f862c66ce5988893ed1e to 1fbe52816c7293bc2e3680d55c389609e5898438
Branch pushed to git repo; I updated commit sha1. New commits:
1fbe528  src/sage/libs/giac/giac.pyx: Update _latex_ doctest

comment:25 Changed 7 weeks ago by
Several errors remain  expert help is needed
sage t randomseed=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/sagerebasing/local/lib/python3.7/sitepackages/sage/doctest/forker.py", line 715, in _run self.compile_and_execute(example, compiler, test.globs) File "/Users/mkoeppe/s/sage/sagerebasing/local/lib/python3.7/sitepackages/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
 Milestone changed from sage9.2 to sage9.3
comment:27 Changed 6 weeks ago by
 Commit changed from 1fbe52816c7293bc2e3680d55c389609e5898438 to 0ad89d8ede5b4e6587c0cf95ab92c17b964caf18
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[]

comment:28 Changed 6 weeks ago by
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 followup: ↓ 32 Changed 6 weeks ago by
Does this also work with earlier 1.5.0.x? Then perhaps we can go forward with this ticket without the upgrade in #29552
comment:30 in reply to: ↑ 4 Changed 6 weeks ago by
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
 Commit changed from 0ad89d8ede5b4e6587c0cf95ab92c17b964caf18 to 17bc2e47342a58de0c73d7593df07c333e2f7dd4
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

comment:32 in reply to: ↑ 29 Changed 6 weeks ago by
 Milestone changed from sage9.3 to sage9.2
 Status changed from needs_work to needs_review
comment:33 Changed 6 weeks ago by
 Dependencies changed from #30277, #29552 to #30277
comment:34 Changed 6 weeks ago by
 Reviewers set to Matthias Koeppe, ...
comment:35 Changed 6 weeks ago by
 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
It would probably be good to add some docstrings, for example for GiacInstance
...
comment:37 Changed 6 weeks ago by
 Commit changed from 17bc2e47342a58de0c73d7593df07c333e2f7dd4 to 2f528a1d31dfb81a7c229cf376a5fcfe9ee677ba
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

comment:38 Changed 6 weeks ago by
This seems to work well. We need another reviewer on this ticket...
comment:39 Changed 6 weeks ago by
 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
 Commit changed from 2f528a1d31dfb81a7c229cf376a5fcfe9ee677ba to 2e44aa6a3ea5e4c2dfc281ce82f2a52a90ca60ce
Branch pushed to git repo; I updated commit sha1. New commits:
2e44aa6  remove optional giacpy_sage from multi_polynomial_ideal

comment:41 followup: ↓ 44 Changed 6 weeks ago by
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 rootdirectory:/home/freddev/sage/develop/sage.new/local/share/giac/ // Giac share rootdirectory:/home/freddev/sage/develop/sage.new/local/share/giac/ Help file /home/freddev/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 followups: ↓ 43 ↓ 45 Changed 6 weeks ago by
I have used this file https://gitlab.math.univparisdiderot.fr/han/giacpysage/blob/master/mkkeywords.py to create the automethods.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 automethods.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
Replying to frederichan:
I have used this file https://gitlab.math.univparisdiderot.fr/han/giacpysage/blob/master/mkkeywords.py to create the automethods.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 automethods.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
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 rootdirectory:/home/freddev/sage/develop/sage.new/local/share/giac/ Giac share rootdirectory:/home/freddev/sage/develop/sage.new/local/share/giac/
Perhaps open an issue with upstream to reduce verbosity in library mode?
Help file /home/freddev/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
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
 Commit changed from 2e44aa6a3ea5e4c2dfc281ce82f2a52a90ca60ce to db48d99b8f5f85aa7794e7447feec89ae267d2e6
Branch pushed to git repo; I updated commit sha1. New commits:
db48d99  add a lazy import for libgiac

comment:47 followup: ↓ 48 Changed 6 weeks ago by
Ok so I have added giacpymkeywords.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
Replying to frederichan:
Ok so I have added giacpymkeywords.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
Ready for "positive review" from my side...
comment:50 Changed 6 weeks ago by
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
 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
 Status changed from positive_review to needs_work
sage t long warnlong 60.7 randomseed=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
 Commit changed from db48d99b8f5f85aa7794e7447feec89ae267d2e6 to 911ddfbe2fd809473e82eaf0bac1a4c50858d161
comment:54 Changed 5 weeks ago by
 Status changed from needs_work to needs_review
Cherrypicked one commit from #29541
comment:55 Changed 5 weeks ago by
Quick review?
comment:56 Changed 5 weeks ago by
 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
Thanks
comment:58 Changed 5 weeks ago by
 Branch changed from public/29171 to 911ddfbe2fd809473e82eaf0bac1a4c50858d161
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
2917: move giacpy_sage into Sage source code