#32386 closed enhancement (fixed)
Merge pynac sources as src/sage/symbolic/ginac
Reported by:  mkoeppe  Owned by:  

Priority:  major  Milestone:  sage9.5 
Component:  symbolics  Keywords:  
Cc:  ghkliem, fbissey, arojas, ghDaveWitteMorris, slelievre, dimpase, tscrim, egourgoulhon, nbruin  Merged in:  
Authors:  Matthias Koeppe, Jonathan Kliem  Reviewers:  Dima Pasechnik 
Report Upstream:  N/A  Work issues:  
Branch:  0d0b58f (Commits, GitHub, GitLab)  Commit:  
Dependencies:  #32391, #32407, #32461  Stopgaps: 
Description (last modified by )
Pynac has a compiletime dependency on the Python library but is not installed using Python package infrastructure. This is problematic because Python users cannot install it using standard Python tools  for example for testing different Python versions.
Pynac has no other uses than as the core of the symbolic expressions facility of Sage; and cannot even be tested without Sage.
We merge Pynac (i.e., the directory https://github.com/pynac/pynac/tree/master/ginac) into the Sage library as src/sage/symbolic/ginac
. It is not a very big package  one flat directory with 100 *.h and *.cpp files, a total of 50kLOC. (The existing Python/Cython symbolics code in sage.symbolic
, sage.calculus
, sage.functions
is about 60kLOC.)
The Pynac sources are compiled like other C++ sources that are already in the Sage source tree. They are linked into a single Python extension module, sage.symbolic.expression
. All other extension modules that used to link to pynac (sage.libs.pynac.pynac
, sage.symbolic.function
, sage.symbolic.series
etc.) are
 either refactored so that they only
import
helper functions provided bysage.symbolic.expression
instead of having tocimport
Pynac functions directly (see #32391, #32407)  or merged into the extension module
sage.symbolic.expression
(via Cythoninclude
statements, not by copypaste of source code, to keep the diff small)
This solves the same issues that #30534 tried to address, which ran into unresolved technical difficulties.
It will also make it easier for Sage developers to make changes to Pynac and the related symbolic implementation.
Followup: #32387 Remove pynac spkg
Change History (115)
comment:1 Changed 9 months ago by
 Description modified (diff)
 Summary changed from Merge pynac as src/sage/symbolics/ginac to Merge pynac as src/sage/symbolic/ginac
comment:2 Changed 9 months ago by
 Branch set to u/mkoeppe/merge_pynac_as_src_sage_symbolic_ginac
comment:3 Changed 9 months ago by
 Commit set to e43830d72c9a45a60f9959fe8a0bbb43f8d39256
comment:4 Changed 9 months ago by
 Commit changed from e43830d72c9a45a60f9959fe8a0bbb43f8d39256 to 2bbf412594790a63b81855f9f5d467c98e4b3a54
Branch pushed to git repo; I updated commit sha1. New commits:
2bbf412  build/pkgs/pynac: Remove

comment:5 Changed 9 months ago by
 Cc tscrim added
Next: Move stuff from sage.libs.pynac
to sage.symbolic.ginac
, leaving some deprecated reimports behind.
Help from Cython experts very welcome...
comment:6 Changed 9 months ago by
 Commit changed from 2bbf412594790a63b81855f9f5d467c98e4b3a54 to e693827e4e554469a35b62ba0c9e68f2a24d3503
Branch pushed to git repo; I updated commit sha1. New commits:
e693827  Move stuff from sage.libs.pynac to sage.symbolic.ginac, leaving some deprecated reimports behind

comment:7 Changed 9 months ago by
 Commit changed from e693827e4e554469a35b62ba0c9e68f2a24d3503 to 0581bac4e5cae56162ef7ff2bb55081434054414
comment:8 Changed 9 months ago by
This version compiles OK
comment:9 Changed 9 months ago by
 Summary changed from Merge pynac as src/sage/symbolic/ginac to Merge pynac sources as src/sage/symbolic/ginac
comment:10 Changed 9 months ago by
 Commit changed from 0581bac4e5cae56162ef7ff2bb55081434054414 to fc29c1de46013777a5e4e2b592b9e0e0cf3e21d0
comment:11 Changed 9 months ago by
 Commit changed from fc29c1de46013777a5e4e2b592b9e0e0cf3e21d0 to add5e891706dd779b6129cea0e8dc175c3c48054
comment:12 Changed 9 months ago by
 Status changed from new to needs_review
comment:13 Changed 9 months ago by
 Description modified (diff)
comment:14 Changed 9 months ago by
This mostly works but I'm getting a few timeouts/crashes that I'm not sure about
sage t randomseed=0 src/sage/schemes/elliptic_curves/ell_number_field.py # Timed out sage t randomseed=0 src/sage/manifolds/differentiable/degenerate_submanifold.py # 1 doctest failed sage t randomseed=0 src/sage/manifolds/differentiable/tensorfield.py # Timed out sage t randomseed=0 src/sage/plot/plot3d/parametric_plot3d.py # Timed out sage t randomseed=0 src/sage/plot/plot3d/plot3d.py # Timed out sage t randomseed=0 src/sage/plot/plot3d/base.pyx # Timed out sage t randomseed=0 src/sage/plot/plot3d/implicit_plot3d.py # Timed out sage t randomseed=0 src/sage/symbolic/relation.py # Timed out sage t randomseed=0 src/sage/symbolic/expression.pyx # Killed due to illegal instruction sage t randomseed=0 src/sage/symbolic/pynac_constant.pyx # 18 doctests failed
comment:15 Changed 9 months ago by
 Commit changed from add5e891706dd779b6129cea0e8dc175c3c48054 to d9a14207300cc7baece7289cf309e7db1bfdebe1
Branch pushed to git repo; I updated commit sha1. New commits:
d9a1420  src/sage/symbolic/pynac_constant.pyx: Fix imports in doctests

comment:16 Changed 9 months ago by
The crash is from this doctest:
>>> m = var('m') >>> assume(m, 'integer') >>> (I**m).real_part()
which seems to lead to an infinite recursion through GiNaC::power::real_part()
comment:17 Changed 9 months ago by
Oh, this is #28357 and I forgot to apply realpartloop.patch
comment:18 Changed 9 months ago by
 Commit changed from d9a14207300cc7baece7289cf309e7db1bfdebe1 to bc0d5f379151013fdcfd942fc0fea030feb11456
Branch pushed to git repo; I updated commit sha1. New commits:
bc0d5f3  Trac #28357: patch power::real_part

comment:19 followup: ↓ 21 Changed 9 months ago by
Ready for testing and review. (I think the patchbot will not run because build/pkgs/pynac/
is removed by the ticket.)
comment:20 Changed 9 months ago by
 Commit changed from bc0d5f379151013fdcfd942fc0fea030feb11456 to 881e21ba0f445e4e257c72c4415894023cab7826
Branch pushed to git repo; I updated commit sha1. New commits:
881e21b  .gitignore: Unignore ginac cpp sources

comment:21 in reply to: ↑ 19 Changed 9 months ago by
Replying to mkoeppe:
Ready for testing and review. (I think the patchbot will not run because
build/pkgs/pynac/
is removed by the ticket.)
That's a bit inconvenient. Such a massive change needs all the bot runs it can get.
comment:22 Changed 9 months ago by
I can of course move the pynac removal to a followup ticket
comment:23 Changed 9 months ago by
 Commit changed from 881e21ba0f445e4e257c72c4415894023cab7826 to 17d6d2d5e8cb021481d4a4b1d483d924097e504b
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
8c1fd8d  .gitignore: Unignore ginac cpp sources

c8c3f53  Trac #28357: patch power::real_part

e117724  Move stuff from sage.libs.pynac to sage.symbolic.ginac, leaving some deprecated reimports behind

e166523  src/sage/symbolic/ginac/version.h.in: Remove

3e428de  src/sage/symbolic/ginac/pynacconfig.h: New

dec1c40  src/sage/symbolic/ginac/utils.h, pynacconfig.h: Eliminate use of PYNAC_SIZEOF_LONG, PYNAC_SIZEOF_LONG_LONG

17d6d2d  src/sage/symbolic/pynac_constant.pyx: Fix imports in doctests

comment:24 Changed 9 months ago by
 Description modified (diff)
comment:25 Changed 9 months ago by
 Status changed from needs_review to needs_work
My previous tests were on macOS. On Linux this fails with
File "sage/symbolic/pynac.pyx", line 1, in init sage.symbolic.pynac (build/cythonized/sage/symbolic/pynac.cpp:31348) ImportError: /home/sagemath/sage9.1/local/lib/python3.7/sitepackages/sage/symbolic/expression.cpython37mx86_64linuxgnu.so: undefined symbol: _ZTIN5GiNaC5basicE
(as reported by a patchbot; also verified using tox e dockerubuntufocalstandard
)
comment:26 Changed 9 months ago by
 Commit changed from 17d6d2d5e8cb021481d4a4b1d483d924097e504b to 3355234a8d74da9413d18c44dd987897d05ed0fe
comment:27 followup: ↓ 37 Changed 9 months ago by
This problem with linking can likely be solved by merging all *.pyx files that cimport
from sage.symbolic.pynac
into one (taking care of the ones in ring.pyx
by refactoring)
$ git grep pynac.*cimport src/sage/symbolic/comparison.pyx:from sage.symbolic.pynac cimport ( src/sage/symbolic/expression.pxd:from sage.symbolic.pynac cimport GEx src/sage/symbolic/expression.pyx:from sage.symbolic.pynac cimport * src/sage/symbolic/function.pyx:from sage.symbolic.pynac cimport ( src/sage/symbolic/getitem.pyx:from sage.symbolic.pynac cimport GEx src/sage/symbolic/pynac.pyx:from .pynac_constant cimport PynacConstant src/sage/symbolic/pynac_constant.pxd:from .pynac cimport GConstant src/sage/symbolic/pynac_constant.pyx:from .pynac cimport ( src/sage/symbolic/ring.pyx:from sage.symbolic.pynac cimport (GEx, GExprSeq, GExVector, GSymbol, src/sage/symbolic/series.pyx:from sage.symbolic.pynac cimport GEx, g_is_a_terminating_series, g_series_var, series_to_poly src/sage/symbolic/substitution_map.pxd:from sage.symbolic.pynac cimport GExMap src/sage/symbolic/substitution_map.pyx:from sage.symbolic.pynac cimport make_pair, GEx, GExPair
I don't know if there's a better solution.
comment:28 Changed 9 months ago by
I will take a look, if I can resolve this.
comment:29 Changed 9 months ago by
 Branch changed from u/mkoeppe/merge_pynac_as_src_sage_symbolic_ginac to u/ghkliem/merge_pynac_as_src_sage_symbolic_ginac
 Commit changed from 3355234a8d74da9413d18c44dd987897d05ed0fe to 04397d2e547f0a81c727165d9e9fee0d54b3ee6d
comment:30 Changed 9 months ago by
 Commit changed from 04397d2e547f0a81c727165d9e9fee0d54b3ee6d to 70bcddf168950f477c2b56fca725e668a1236fb6
Branch pushed to git repo; I updated commit sha1. New commits:
70bcddf  simpler inclusion of headers

comment:31 Changed 9 months ago by
 Commit changed from 70bcddf168950f477c2b56fca725e668a1236fb6 to 7ce050fdea338610b91680584c0db59e54d4c5cf
Branch pushed to git repo; I updated commit sha1. New commits:
7ce050f  somewhat fix doctest

comment:32 Changed 9 months ago by
I basically have this working by specifying:

src/sage/symbolic/series.pyx
diff git a/src/sage/symbolic/series.pyx b/src/sage/symbolic/series.pyx index 0e395ae4b4..d93cccc50b 100644
a b 1 # distutils: extra_link_args = /srv/public/kliem/sage/local/lib/python3.7/sitepackages/sage/symbolic/pynac.cpython37mx86_64linuxgnu.so 1 2 """ 2 3 Symbolic Series
to all of those files (and the one doctest in sage/symbolic/pynac.pxd
that raises an ImportError
currently).
So this is basically working, but the correct linking is missing.
We could create an extra file sage/symbolic/linked_pynac.pxd
, which just consists of
# distutils: extra_link_args = ... from sage.symbolic/pynac cimport *
then there is no need to specify this extra distutils thing in every file that cimports pynac.
comment:33 followup: ↓ 36 Changed 9 months ago by
pkgs/sagemathstandard/build/cythonized/
are not cleaned up. Can this be fixed somehow or is this just going to stay this way? (This is probably the reason, why you did not notice the missing headers that were fixed in 01dfef3  missing headers

comment:34 followup: ↓ 39 Changed 9 months ago by
I don't like the change https://git.sagemath.org/sage.git/commit/?id=70bcddf168950f477c2b56fca725e668a1236fb6 (simpler inclusion of headers); putting stuff in the include search path can lead to shadowing headers of other libraries
comment:35 Changed 9 months ago by
Thanks for the investigation and fixes.
I think I would be reluctant to use the extra_link_args
workaround  it will requires changes to the build system because now building one extension module would depend on another extension module.
In any case, I'd think that your successful experiment indicates the approach of merging everything that needs to cimport
stuff from ginac/ into one extension module would work.
comment:36 in reply to: ↑ 33 Changed 9 months ago by
comment:37 in reply to: ↑ 27 ; followup: ↓ 38 Changed 9 months ago by
 Dependencies set to #32391
comment:38 in reply to: ↑ 37 ; followup: ↓ 40 Changed 9 months ago by
Replying to mkoeppe:
Replying to mkoeppe:
This problem with linking can likely be solved by merging all *.pyx files that
cimport
fromsage.symbolic.pynac
into one (taking care of the ones inring.pyx
by refactoring)For cleaning up
ring.pyx
, I have opened #32391.
We would still lose ability to use it directly in cython at all. E.g. the doctest in pynac.pxd
would fail.
At the moment, I'm building this branch with ./configure enableeditable
and will report whether this resolves anything.
comment:39 in reply to: ↑ 34 Changed 9 months ago by
Replying to mkoeppe:
I don't like the change https://git.sagemath.org/sage.git/commit/?id=70bcddf168950f477c2b56fca725e668a1236fb6 (simpler inclusion of headers); putting stuff in the include search path can lead to shadowing headers of other libraries
from ... cimport ...
should usually just work out of the box. But of course one can also spell out all those header (e.g. with a cython alias).
Then again, if we decide to put it all into one large pyx
and not cimport things from ginac
directly, the commit isn't necessary at all.
comment:40 in reply to: ↑ 38 Changed 9 months ago by
Replying to ghkliem:
Replying to mkoeppe:
Replying to mkoeppe:
This problem with linking can likely be solved by merging all *.pyx files that
cimport
fromsage.symbolic.pynac
into one (taking care of the ones inring.pyx
by refactoring)For cleaning up
ring.pyx
, I have opened #32391.We would still lose ability to use it directly in cython at all. E.g. the doctest in
pynac.pxd
would fail.
I wouldn't be too concerned about this. I'd be surprised if there's any user code in the wild that does that; and if at all necessary, it can be mitigated by adding some missing methods at a higher level.
comment:41 Changed 9 months ago by
Ok, same problem with ./configure enableeditable
.
Btw, this is exactly our problem and there is no proper solution to it. So instead of adding a custom one, I agree that it would be easier, to move all the cdef extern from ...
into one large pyx
and only cimport
the cython wrappers. This is the way, your meant to do it anyway.
comment:42 Changed 9 months ago by
A simple way of doing this might be to rename files like series.pxd
to series.pxi
and series.pyx
to series_impl.pxi
and then to include
them into the target extension module (pynac.pxd
, pynac.pyx
?)
comment:43 Changed 9 months ago by
Using pxi
is outdated, I was told. But maybe using something outdated is more desirable than having huge files.
comment:44 Changed 9 months ago by
That's probably just from the warning at https://cython.readthedocs.io/en/latest/src/userguide/language_basics.html?highlight=include#theincludestatementandincludefiles; but if I'm not mistaken, this just refers to a particular use case of "pxi" that was superseded by "pxd". I don't see any indication that the mechanism of include
is intended to be removed in Cython.
comment:45 Changed 9 months ago by
 Branch changed from u/ghkliem/merge_pynac_as_src_sage_symbolic_ginac to u/mkoeppe/merge_pynac_as_src_sage_symbolic_ginac
comment:46 Changed 9 months ago by
 Commit changed from 7ce050fdea338610b91680584c0db59e54d4c5cf to 188714b3bb4f953e1fb56c2841899e1e946d5900
comment:47 Changed 9 months ago by
(cherrypicked some of your changes onto this branch)
comment:48 Changed 9 months ago by
 Commit changed from 188714b3bb4f953e1fb56c2841899e1e946d5900 to 57ff18ac158d3c2541332652314aab9cdfa9d900
Branch pushed to git repo; I updated commit sha1. New commits:
57ff18a  Make sage.symbolic.expression the extension module that includes pynac

comment:49 Changed 9 months ago by
or perhaps better like this.
comment:50 Changed 9 months ago by
 Commit changed from 57ff18ac158d3c2541332652314aab9cdfa9d900 to 6a2ada278e567b7fafd06ce97cad895e1f323f95
Branch pushed to git repo; I updated commit sha1. New commits:
6a2ada2  src/sage/symbolic/expression.pyx: Move 'distutils: sources' here

comment:51 followup: ↓ 52 Changed 9 months ago by
Forgive me for asking a naïve question, but isn't this going against the modularization you are trying to have Sage do? I fully understand the reason for bringing it in (which should make it easier to bugfix), but doesn't this make Sage more monolithic?
comment:52 in reply to: ↑ 51 Changed 9 months ago by
Replying to tscrim:
isn't this going against the modularization you are trying to have Sage do? I fully understand the reason for bringing it in (which should make it easier to bugfix), but doesn't this make Sage more monolithic?
This is a natural question.
The pynac library just does not make sense as a module. It is the basis of the implementation of SR elements and as such there is no way to build Sage symbolics without it.
In the modularization plan of #29705, we would instead create a distribution package sagemathsymbolics
, containing the integrated pynac from the present ticket and most of sage.symbolic
, sage.calculus
, sage.functions
, and parts of sage.interfaces
. This will make sense as a module because there are many parts of Sage that (fortunately!) do not depend at all on Sage symbolics, or only depend on it for some smaller features. For example, sage.graphs
has 0 imports from sage.symbolic
, sage.combinat
just a handful, etc.
comment:53 followup: ↓ 54 Changed 9 months ago by
Thank you for the explanation. So wouldn't it make sense to instead take the symbolics stuff out from Sage in one go rather than this two step process of add in pynac just to remove it later? Or is this just meant to be a stopgap until it comes time to split everything?
comment:54 in reply to: ↑ 53 Changed 9 months ago by
Replying to tscrim:
So wouldn't it make sense to instead take the symbolics stuff out from Sage in one go rather than this two step process of add in pynac just to remove it later? Or is this just meant to be a stopgap until it comes time to split everything?
The modularization plan does not involve "taking ... stuff out from Sage" at all. It will all remain in the same git repository, and also src/sage
will remain monolithic.
Modularization is achieved by preparing subdistributions defined using one of several possible mechanisms. Take a look at sagemathobjects
from #29865 (https://pypi.org/project/sagemathobjects/), for example: It defines a subdistribution using a MANIFEST
file. Via the Python 3 "native namespace packages" mechanism, (after some work in #31420, #28925), these distributions can be installed cleanly next to each other and provide the shared sage.*
namespace.
comment:55 Changed 9 months ago by
 Dependencies changed from #32391 to #32391, #32407
comment:56 Changed 9 months ago by
 Commit changed from 6a2ada278e567b7fafd06ce97cad895e1f323f95 to 759aa5d012d2ee2924456cb8df33514e351d2ffb
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
ef22257  src/sage/symbolic/function.pyx: Undo rename

4ac54df  src/sage/symbolic/function.pyx: Replace cimport * by more specific cimport

6f49aee  sage.symbolic.function.Function.__call__: Refactor through new helper function sage.symbolic.expression.call_by_ginac_serial

2f92b90  sage.symbolic.function.Function._register_function: Refactor through new function sage.symbolic.expression.register_or_update_function

e81e6c7  sage.symbolic.function.GinacFunction._register_function: Refactor through sage.symbolic.expression.register_or_update_function

d397ef1  sage.symbolic.function: Remove unused imports, cimports

1c43dbe  sage.symbolic.function.{GinacFunction,BuiltinFunction}._is_registered: Refactor through new function sage.symbolic.expression.find_registered_function; remove last pynac import

9d4fe5f  sage.symbolic.function: Remove unused cimport

9a9fdc7  Merge #32407

759aa5d  sage.symbolic.expression: Finish merging code using pynac into this one DSO

comment:57 Changed 9 months ago by
It remains to remove the import cycle sage.symbolic.ring
> sage.symbolic.expression
> sage.symbolic.function
> sage.symbolic.ring
comment:58 Changed 9 months ago by
Ah, okay, I was thinking the modularization meant splitting Sage up into different subprojects that would have a main link that would be the actual Sage. However, it is a little different than that. I don't completely understand how all of this works and some of the benefits, but it doesn't seem to be as fragmented as I was thinking. Thank you.
comment:59 Changed 9 months ago by
 Commit changed from 759aa5d012d2ee2924456cb8df33514e351d2ffb to 46c4a6b0b6815076d8f0f5925459bb221f2c408b
comment:60 Changed 9 months ago by
 Commit changed from 46c4a6b0b6815076d8f0f5925459bb221f2c408b to 903792a901e783b70244efa2f8ed6cb4f1eaf454
Branch pushed to git repo; I updated commit sha1. New commits:
903792a  src/sage/symbolic/pynac_constant.py: New

comment:61 Changed 9 months ago by
 Description modified (diff)
comment:62 Changed 9 months ago by
 Commit changed from 903792a901e783b70244efa2f8ed6cb4f1eaf454 to 9f56aa26d702555868d3ba6d9b9338fc19aac390
Branch pushed to git repo; I updated commit sha1. New commits:
9f56aa2  sage.symbolic.ring: cimport the helper functions from sage.symbolic.expression (safe because expression.pxd no longer declares the Expression class)

comment:63 Changed 9 months ago by
 Commit changed from 9f56aa26d702555868d3ba6d9b9338fc19aac390 to 3153781f5e6c02b9a1a0cef9f8295f206a702162
Branch pushed to git repo; I updated commit sha1. New commits:
3153781  WIP: Try to break import cycle

comment:64 Changed 9 months ago by
Still struggling with an import cycle sage.symbolic.ring
> sage.symbolic.expression
> sage.symbolic.ring
comment:65 Changed 9 months ago by
Help welcome
comment:66 Changed 9 months ago by
I recall that i
, a.k.a. I
, is really close the the center of the import troubles.
comment:67 Changed 9 months ago by
 Branch changed from u/mkoeppe/merge_pynac_as_src_sage_symbolic_ginac to u/ghkliem/merge_pynac_as_src_sage_symbolic_ginac
 Commit changed from 3153781f5e6c02b9a1a0cef9f8295f206a702162 to 7ce050fdea338610b91680584c0db59e54d4c5cf
comment:68 Changed 9 months ago by
 Commit changed from 7ce050fdea338610b91680584c0db59e54d4c5cf to 48267a7cc19287bd9523370ea9ebbda6d3f1085b
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
9a9fdc7  Merge #32407

759aa5d  sage.symbolic.expression: Finish merging code using pynac into this one DSO

b640014  Replace import from sage.symbolic.pynac by sage.symbolic.expression

46c4a6b  sage.symbolic.function: Move sfunction_serial_dict, get_sfunction_from_serial to .expression, refactor

903792a  src/sage/symbolic/pynac_constant.py: New

9f56aa2  sage.symbolic.ring: cimport the helper functions from sage.symbolic.expression (safe because expression.pxd no longer declares the Expression class)

3153781  WIP: Try to break import cycle

a56c69d  fix startup

5b557c6  deprecation warnings

48267a7  fix doctests

comment:69 Changed 9 months ago by
 Status changed from needs_work to needs_review
Seems to work now.
comment:70 Changed 9 months ago by
 Commit changed from 48267a7cc19287bd9523370ea9ebbda6d3f1085b to 5776163f25a6a26bb0063b56ad01517907c00b3a
Branch pushed to git repo; I updated commit sha1. New commits:
5776163  wrong import

comment:71 Changed 9 months ago by
Cool, thanks very much!!
comment:72 followup: ↓ 77 Changed 9 months ago by
I'm getting
sage t randomseed=0 src/sage/functions/prime_pi.pyx ********************************************************************** File "src/sage/functions/prime_pi.pyx", line 17, in sage.functions.prime_pi Failed example: loads(dumps(z)) == z Expected: True Got: False
comment:73 followups: ↓ 84 ↓ 114 Changed 9 months ago by
 Cc egourgoulhon added
It will also need to be checked that the deferred imports (such as those introduced in my commit 3153781) do not lead to speed regressions
comment:74 Changed 9 months ago by
Also I don't think we need src/sage/symbolic/pynac.py
 the module sage.symbolic.pynac
only had a shortlived existence here in this branch.
Did you mean to add these lazy_import
to src/sage/libs/pynac/pynac.py
?
comment:75 Changed 9 months ago by
 Branch changed from u/ghkliem/merge_pynac_as_src_sage_symbolic_ginac to u/mkoeppe/merge_pynac_as_src_sage_symbolic_ginac
comment:76 Changed 9 months ago by
 Branch changed from u/mkoeppe/merge_pynac_as_src_sage_symbolic_ginac to u/ghkliem/merge_pynac_as_src_sage_symbolic_ginac
 Commit changed from 5776163f25a6a26bb0063b56ad01517907c00b3a to a27d820bb00ddcd82353eec55eab8920920068b7
New commits:
c12e71e  moved imports to libs/pynac/pynac.py

b898086  fix serial getting of BuiltinFunction

7e17150  fixed import in doctest

4ec43a2  src/sage/symbolic/ginac/README.md: New

a27d820  Merge branch 'u/mkoeppe/merge_pynac_as_src_sage_symbolic_ginac' of git://trac.sagemath.org/sage into u/ghkliem/merge_pynac_as_src_sage_symbolic_ginac

comment:77 in reply to: ↑ 72 Changed 9 months ago by
Replying to mkoeppe:
I'm getting
sage t randomseed=0 src/sage/functions/prime_pi.pyx ********************************************************************** File "src/sage/functions/prime_pi.pyx", line 17, in sage.functions.prime_pi Failed example: loads(dumps(z)) == z Expected: True Got: False
Fixed.
comment:78 Changed 9 months ago by
 Branch changed from u/ghkliem/merge_pynac_as_src_sage_symbolic_ginac to u/mkoeppe/merge_pynac_as_src_sage_symbolic_ginac
comment:79 Changed 9 months ago by
 Commit changed from a27d820bb00ddcd82353eec55eab8920920068b7 to 2d2153446fc3bb4d2d17d32cc770c0efc5121ecf
Branch pushed to git repo; I updated commit sha1. New commits:
2d21534  src/sage/symbolic/expression.pyx: Update copyright according to 'git blame w date=format:%Y src/sage/symbolic/expression.pyx  sort k2'

comment:80 Changed 9 months ago by
If we want to deprecate these reimports like you do here...
+++ b/src/sage/symbolic/series.py @@ 0,0 +1,2 @@ +from sage.misc.lazy_import import lazy_import +lazy_import('sage.symbolic.expression', 'SymbolicSeries', deprecation=32386)
... then we should probably also do this in substitution_map.py
(and perhaps other places).
comment:81 Changed 9 months ago by
I tried to get all places. I didn't do substitution_map.py
because, you had already added the reimport.
I did those lazy imports here, so that people importing python functions from places where they used to be, get a proper deprecation warning:
comment:82 Changed 9 months ago by
 Commit changed from 2d2153446fc3bb4d2d17d32cc770c0efc5121ecf to 03a4b83be3c6379679cb1b138ee440f60db2710c
Branch pushed to git repo; I updated commit sha1. New commits:
03a4b83  src/sage/symbolic/function.pyx: Update copyright according to 'git blame w date=format:%Y src/sage/symbolic/function.pyx  sort k2'

comment:83 Changed 9 months ago by
 Commit changed from 03a4b83be3c6379679cb1b138ee440f60db2710c to 53453b4d897219e536450651225fb2d2a0b71e92
Branch pushed to git repo; I updated commit sha1. New commits:
53453b4  src/sage/symbolic/*_impl.pxi: Update copyright according to git blame

comment:84 in reply to: ↑ 73 Changed 9 months ago by
 Status changed from needs_review to needs_work
Replying to mkoeppe:
It will also need to be checked that the deferred imports (such as those introduced in my commit 3153781) do not lead to speed regressions
A quick comparison using ./sage tp long src/sage/symbolic/ src/sage/functions/ src/sage/calculus/ src/sage/manifolds/
shows a serious slowdown around +30% with this branch
comment:85 Changed 9 months ago by
I cannot reproduce these timings any more; perhaps this was just from CPU throttling.
comment:86 Changed 9 months ago by
 Status changed from needs_work to needs_review
comment:87 Changed 9 months ago by
 Branch changed from u/mkoeppe/merge_pynac_as_src_sage_symbolic_ginac to u/ghkliem/merge_pynac_as_src_sage_symbolic_ginac
 Commit changed from 53453b4d897219e536450651225fb2d2a0b71e92 to e79cc47188db3896891d951f63973994d64251be
New commits:
e79cc47  sporadic failing tests

comment:88 Changed 9 months ago by
 Branch changed from u/ghkliem/merge_pynac_as_src_sage_symbolic_ginac to u/mkoeppe/merge_pynac_as_src_sage_symbolic_ginac
comment:89 Changed 9 months ago by
 Commit changed from e79cc47188db3896891d951f63973994d64251be to ac9c9594cc1d0e12cb91214a9be3d8c1ec293e4d
Branch pushed to git repo; I updated commit sha1. New commits:
ac9c959  sage.symbolic.pynac_constant: Use lazy_import with deprecation

comment:90 Changed 9 months ago by
 Commit changed from ac9c9594cc1d0e12cb91214a9be3d8c1ec293e4d to e9c8637392c1a811b53b4d9ae21799d02b38a8c8
Branch pushed to git repo; I updated commit sha1. New commits:
d680c35  sage.symbolic.expression.call_registered_function: Rename from call_by_ginac_serial

1f761d8  src/sage/symbolic/pynac_function_impl.pxi: Split out from src/sage/symbolic/expression.pyx

47adbcc  src/sage/symbolic/pynac_function_impl.pxi: Add doctests

1d07a02  Merge #32407

e9c8637  get_sfunction_from_hash: Add doctest

comment:91 followup: ↓ 92 Changed 9 months ago by
If I understand well, that means that when sage 9.5 enters Debian pynac can be let go.
Is that right?
(Asking as the one responsible for pynac in Debian: package tracker)
comment:92 in reply to: ↑ 91 Changed 9 months ago by
Replying to Snark:
If I understand well, that means that when sage 9.5 enters Debian pynac can be let go.
Is that right?
(Asking as the one responsible for pynac in Debian: package tracker)
Yes, you understand well.
comment:93 Changed 9 months ago by
sage t long randomseed=0 src/sage/misc/sageinspect.py ********************************************************************** File "src/sage/misc/sageinspect.py", line 2263, in sage.misc.sageinspect.sage_getsourcelines Failed example: lines, lineno = sage_getsourcelines(x); lines[0:2] Expected: ['cdef class Expression(CommutativeRingElement):\n', ' cpdef object pyobject(self):\n'] Got: ['cdef class Expression(CommutativeRingElement):\n', '\n']
comment:94 Changed 9 months ago by
 Branch changed from u/mkoeppe/merge_pynac_as_src_sage_symbolic_ginac to u/ghkliem/merge_pynac_as_src_sage_symbolic_ginac
 Commit changed from e9c8637392c1a811b53b4d9ae21799d02b38a8c8 to 712f9261c25496053ae203ec6b12b6410560f01c
comment:95 Changed 9 months ago by
 Commit changed from 712f9261c25496053ae203ec6b12b6410560f01c to 5d062ed36ce07dde3c9ba0b3ae7e39e825a6a1b2
comment:96 Changed 9 months ago by
Coverage is still an issue, e.g. new_Expression_from_pyobject
doesn't even have a docstring.
comment:97 Changed 9 months ago by
 Branch changed from u/ghkliem/merge_pynac_as_src_sage_symbolic_ginac to u/mkoeppe/merge_pynac_as_src_sage_symbolic_ginac
comment:98 Changed 9 months ago by
 Commit changed from 5d062ed36ce07dde3c9ba0b3ae7e39e825a6a1b2 to 3f1ac2e8a415de7612f3b9c5ad82165761b61fad
I've solved this by consolidating two similar functions into one
New commits:
3f1ac2e  sage.symbolic.expression: Merge new_Expression_from_pyobject and new_Expression_force_pyobject

comment:99 Changed 9 months ago by
 Reviewers set to https://github.com/mkoeppe/sage/actions/runs/1168763794, https://github.com/mkoeppe/sage/actions/runs/1168763799, ...
comment:100 Changed 9 months ago by
 Commit changed from 3f1ac2e8a415de7612f3b9c5ad82165761b61fad to 0643756930700a165b83987dc9d51e6105e2945a
Branch pushed to git repo; I updated commit sha1. New commits:
0643756  new_Expression_from_pyobject: Add output for new doctests

comment:101 Changed 9 months ago by
 Commit changed from 0643756930700a165b83987dc9d51e6105e2945a to 327ca5f3f5ca2c28d0c66685a2b528c88c10d265
comment:102 Changed 9 months ago by
The portability tests on GH Actions ran through successfully.
Ready for review
comment:103 Changed 9 months ago by
 Cc nbruin added
comment:104 Changed 9 months ago by
there is a failing test:
e023915d474 (Markus Wageringel 20210615 19:54:54 +0200 340) sage: result = integral(sinh(x^2+sqrt(x1)),x) # long time (15s on sage.math, 2012) e023915d474 (Markus Wageringel 20210615 19:54:54 +0200 341) ... e023915d474 (Markus Wageringel 20210615 19:54:54 +0200 342) sage: result d0c52e4aa36 (JeanPierre Flori 20130527 15:36:24 +0200 343) integrate(sinh(x^2 + sqrt(x  1)), x)
line 342 needs # long time
tag.
comment:105 Changed 9 months ago by
Unrelated to this ticket but yes
comment:106 Changed 9 months ago by
opened #32461 for this
comment:107 Changed 9 months ago by
 Reviewers changed from https://github.com/mkoeppe/sage/actions/runs/1168763794, https://github.com/mkoeppe/sage/actions/runs/1168763799, ... to Dima Pasechnik
 Status changed from needs_review to positive_review
lgtm
comment:108 Changed 9 months ago by
Thank you!
comment:109 Changed 9 months ago by
should #32461 be a dependency here?
comment:110 Changed 9 months ago by
 Dependencies changed from #32391, #32407 to #32391, #32407, #32461
comment:111 Changed 8 months ago by
 Commit changed from 327ca5f3f5ca2c28d0c66685a2b528c88c10d265 to 0d0b58f65c0ca01d51756798d6910efb5f26f47c
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
0d0b58f  Merge tag '9.5.beta1' into t/32386/merge_pynac_as_src_sage_symbolic_ginac

comment:112 Changed 8 months ago by
 Status changed from needs_review to positive_review
comment:113 Changed 8 months ago by
 Branch changed from u/mkoeppe/merge_pynac_as_src_sage_symbolic_ginac to 0d0b58f65c0ca01d51756798d6910efb5f26f47c
 Resolution set to fixed
 Status changed from positive_review to closed
comment:114 in reply to: ↑ 73 Changed 8 months ago by
 Commit 0d0b58f65c0ca01d51756798d6910efb5f26f47c deleted
Replying to mkoeppe:
It will also need to be checked that the deferred imports (such as those introduced in my commit 3153781) do not lead to speed regressions
FWIW, the change performed here did not introduced any speed regression in the rather demanding Kerr spacetime notebook, which involves parallelized (8 threads) heavy symbolic computations. On the contrary, it seems there is some little speed up :) On my i78665U laptop:
 Sage 9.4: 461 s
 Sage 9.5.beta2: 449 s
comment:115 Changed 8 months ago by
Thanks for testing!
I ran
git filterbranch f treefilter 'rm f Makefile.am; mkdir p src/sage/symbolic/ginac; mv *.* src/sage/symbolic/ginac/; :'  all
on pynac 0.7.29 and merged it here, thus preserving the relevant historyLast 10 new commits:
Merge pull request #374 from slel/fixexpinvhyp
apply patches from #31479 and #31554
apply trac 31869 fix pynac integer_content
apply trac 30688 patch infinite loop in pynac
apply trac 31645 handling of constant term in series
apply trac 31530 patch pynac power::subs
trac 31585 32bit overflow
also fix /=
ginac/numeric.cpp: Only use builtin for smull_overflow if supported by the compiler
Merge remotetracking branch 'pynacfiltered/master' into t/32386/merge_pynac_as_src_sage_symbolic_ginac