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:  sage9.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: 
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 3 years ago by
Branch:  → public/29171 

Commit:  → 8e49ecf0d50dbc20bc9531bc41d811f8c830ac86 
comment:2 Changed 3 years ago by
Commit:  8e49ecf0d50dbc20bc9531bc41d811f8c830ac86 → 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 3 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 followup: 30 Changed 3 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 3 years ago by
Milestone:  sage9.1 → 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 2 years ago by
Priority:  major → critical 

comment:7 Changed 2 years ago by
Cc:  Han Frederic Markus Wageringel Samuel Lelièvre added 

comment:8 followup: 10 Changed 2 years 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 2 years ago by
Status:  new → needs_info 

comment:10 Changed 2 years 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 2 years 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 2 years ago by
Cc:  François Bissey added 

comment:13 Changed 2 years ago by
Commit:  ac934cbc07f70f8a7d9dbeee7563b57d323cfb8f → 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:15 Changed 2 years ago by
Dependencies:  → #30277 

comment:16 Changed 2 years ago by
Commit:  d095c37d862707b57721fb9c8d85b4b39b7c3b51 → 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 2 years ago by
Authors:  Vincent Delecroix → Vincent Delecroix, Matthias Koeppe 

Status:  needs_info → needs_review 
comment:18 Changed 2 years ago by
Commit:  5d56332fafce16b294eef025855f8af9edce151a → 5471b803be3861e8c2e391c9625647ec49bfa8e5 

comment:19 Changed 2 years ago by
Dependencies:  #30277 → #30277, #29552 

comment:20 Changed 2 years 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 2 years ago by
Commit:  5471b803be3861e8c2e391c9625647ec49bfa8e5 → 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 2 years ago by
Status:  needs_review → needs_work 

comment:23 Changed 2 years ago by
Commit:  6a2934ec8258214cfe2708ce935c01b1f9b99b80 → aff8292282fcf9bab697f862c66ce5988893ed1e 

comment:24 Changed 2 years ago by
Commit:  aff8292282fcf9bab697f862c66ce5988893ed1e → 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 2 years 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 2 years ago by
Milestone:  sage9.2 → sage9.3 

comment:27 Changed 2 years ago by
Commit:  1fbe52816c7293bc2e3680d55c389609e5898438 → 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 2 years 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 2 years 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 Changed 2 years 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 2 years ago by
Commit:  0ad89d8ede5b4e6587c0cf95ab92c17b964caf18 → 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 Changed 2 years ago by
Milestone:  sage9.3 → sage9.2 

Status:  needs_work → needs_review 
comment:33 Changed 2 years ago by
Dependencies:  #30277, #29552 → #30277 

comment:34 Changed 2 years ago by
Authors:  Vincent Delecroix, Matthias Koeppe → Vincent Delecroix, Matthias Koeppe, Frederic Han 

Reviewers:  → Matthias Koeppe, ... 
comment:35 Changed 2 years ago by
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
It would probably be good to add some docstrings, for example for GiacInstance
...
comment:37 Changed 2 years ago by
Commit:  17bc2e47342a58de0c73d7593df07c333e2f7dd4 → 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 2 years ago by
This seems to work well. We need another reviewer on this ticket...
comment:39 Changed 2 years ago by
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
Commit:  2f528a1d31dfb81a7c229cf376a5fcfe9ee677ba → 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 2 years 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 2 years 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 Changed 2 years 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 Changed 2 years 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 Changed 2 years 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 2 years ago by
Commit:  2e44aa6a3ea5e4c2dfc281ce82f2a52a90ca60ce → db48d99b8f5f85aa7794e7447feec89ae267d2e6 

Branch pushed to git repo; I updated commit sha1. New commits:
db48d99  add a lazy import for libgiac

comment:47 followup: 48 Changed 2 years 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 Changed 2 years 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:50 Changed 2 years 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 2 years ago by
Reviewers:  Matthias Koeppe, ... → Matthias Koeppe, Frederic Han 

Status:  needs_review → positive_review 
comment:52 Changed 2 years ago by
Status:  positive_review → 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 2 years ago by
Commit:  db48d99b8f5f85aa7794e7447feec89ae267d2e6 → 911ddfbe2fd809473e82eaf0bac1a4c50858d161 

comment:54 Changed 2 years ago by
Status:  needs_work → needs_review 

Cherrypicked one commit from #29541
comment:56 Changed 2 years ago by
Reviewers:  Matthias Koeppe, Frederic Han → Matthias Koeppe, Frederic Han, Dima Pasechnik 

Status:  needs_review → positive_review 
lgtm
comment:58 Changed 2 years ago by
Branch:  public/29171 → 911ddfbe2fd809473e82eaf0bac1a4c50858d161 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
2917: move giacpy_sage into Sage source code