Opened 2 years ago
Closed 21 months 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, GitHub, GitLab) | 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 2 years ago by
- Branch set to public/29171
- Commit set to 8e49ecf0d50dbc20bc9531bc41d811f8c830ac86
comment:2 Changed 2 years 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 2 years 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 follow-up: ↓ 30 Changed 2 years 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 2 years ago by
- 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 22 months ago by
- Priority changed from major to critical
comment:7 Changed 22 months ago by
- Cc frederichan gh-mwageringel slelievre added
comment:8 follow-up: ↓ 10 Changed 22 months ago by
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 22 months ago by
- Status changed from new to needs_info
comment:10 in reply to: ↑ 8 Changed 22 months ago by
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 22 months 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.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 22 months ago by
- Cc fbissey added
comment:13 Changed 22 months 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 22 months ago by
Rebased on 9.2.beta8
comment:15 Changed 22 months ago by
- Dependencies set to #30277
comment:16 Changed 22 months 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/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
|
comment:17 Changed 22 months ago by
- Status changed from needs_info to needs_review
comment:18 Changed 22 months ago by
- Commit changed from 5d56332fafce16b294eef025855f8af9edce151a to 5471b803be3861e8c2e391c9625647ec49bfa8e5
comment:19 Changed 22 months ago by
- Dependencies changed from #30277 to #30277, #29552
comment:20 Changed 22 months ago by
[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 22 months 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 22 months ago by
- Status changed from needs_review to needs_work
comment:23 Changed 22 months ago by
- Commit changed from 6a2934ec8258214cfe2708ce935c01b1f9b99b80 to aff8292282fcf9bab697f862c66ce5988893ed1e
comment:24 Changed 22 months 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 22 months ago by
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 22 months ago by
- Milestone changed from sage-9.2 to sage-9.3
comment:27 Changed 22 months 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 22 months 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 follow-up: ↓ 32 Changed 22 months 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 22 months 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 22 months 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 22 months ago by
- Milestone changed from sage-9.3 to sage-9.2
- Status changed from needs_work to needs_review
comment:33 Changed 22 months ago by
- Dependencies changed from #30277, #29552 to #30277
comment:34 Changed 22 months ago by
- Reviewers set to Matthias Koeppe, ...
comment:35 Changed 22 months 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 22 months ago by
It would probably be good to add some docstrings, for example for GiacInstance
...
comment:37 Changed 22 months 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 22 months ago by
This seems to work well. We need another reviewer on this ticket...
comment:39 Changed 22 months 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 22 months 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 follow-up: ↓ 44 Changed 22 months 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 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: ↓ 43 ↓ 45 Changed 22 months ago by
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 22 months ago by
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 22 months 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 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 22 months 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 22 months 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 follow-up: ↓ 48 Changed 22 months ago by
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 22 months ago by
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 22 months ago by
Ready for "positive review" from my side...
comment:50 Changed 21 months 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 21 months ago by
- Reviewers changed from Matthias Koeppe, ... to Matthias Koeppe, Frederic Han
- Status changed from needs_review to positive_review
comment:52 Changed 21 months ago by
- 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 21 months ago by
- Commit changed from db48d99b8f5f85aa7794e7447feec89ae267d2e6 to 911ddfbe2fd809473e82eaf0bac1a4c50858d161
comment:54 Changed 21 months ago by
- Status changed from needs_work to needs_review
Cherry-picked one commit from #29541
comment:55 Changed 21 months ago by
Quick review?
comment:56 Changed 21 months 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 21 months ago by
Thanks
comment:58 Changed 21 months 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