Opened 4 months ago

Closed 3 months ago

Last modified 2 months ago

#32386 closed enhancement (fixed)

Merge pynac sources as src/sage/symbolic/ginac

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.5
Component: symbolics Keywords:
Cc: gh-kliem, fbissey, arojas, gh-DaveWitteMorris, 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:

Status badges

Description (last modified by mkoeppe)

Pynac has a compile-time 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 by sage.symbolic.expression instead of having to cimport Pynac functions directly (see #32391, #32407)
  • or merged into the extension module sage.symbolic.expression (via Cython include statements, not by copy-paste 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.

Follow-up: #32387 Remove pynac spkg

Change History (115)

comment:1 Changed 4 months ago by mkoeppe

  • Description modified (diff)
  • Summary changed from Merge pynac as src/sage/symbolics/ginac to Merge pynac as src/sage/symbolic/ginac

comment:2 Changed 4 months ago by mkoeppe

  • Branch set to u/mkoeppe/merge_pynac_as_src_sage_symbolic_ginac

comment:3 Changed 4 months ago by mkoeppe

  • Commit set to e43830d72c9a45a60f9959fe8a0bbb43f8d39256

I ran git filter-branch -f --tree-filter '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 history


Last 10 new commits:

f3bb79fMerge pull request #374 from slel/fix-exp-inv-hyp
a99791bapply patches from #31479 and #31554
91cb6faapply trac 31869 fix pynac integer_content
d96393capply trac 30688 patch infinite loop in pynac
80269c5apply trac 31645 handling of constant term in series
4caca2eapply trac 31530 patch pynac power::subs
cba9038trac 31585 32-bit overflow
8acb548also fix /=
f2040c8ginac/numeric.cpp: Only use builtin for smull_overflow if supported by the compiler
e43830dMerge remote-tracking branch 'pynac-filtered/master' into t/32386/merge_pynac_as_src_sage_symbolic_ginac

comment:4 Changed 4 months ago by git

  • Commit changed from e43830d72c9a45a60f9959fe8a0bbb43f8d39256 to 2bbf412594790a63b81855f9f5d467c98e4b3a54

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

2bbf412build/pkgs/pynac: Remove

comment:5 Changed 4 months ago by mkoeppe

  • 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 4 months ago by git

  • Commit changed from 2bbf412594790a63b81855f9f5d467c98e4b3a54 to e693827e4e554469a35b62ba0c9e68f2a24d3503

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

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

comment:7 Changed 4 months ago by git

  • Commit changed from e693827e4e554469a35b62ba0c9e68f2a24d3503 to 0581bac4e5cae56162ef7ff2bb55081434054414

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

0bfc24fsrc/sage/symbolic/ginac/version.h.in: Remove
0581bacsrc/sage/symbolic/ginac/pynac-config.h: New

comment:8 Changed 4 months ago by mkoeppe

This version compiles OK

comment:9 Changed 4 months ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Summary changed from Merge pynac as src/sage/symbolic/ginac to Merge pynac sources as src/sage/symbolic/ginac

comment:10 Changed 4 months ago by git

  • Commit changed from 0581bac4e5cae56162ef7ff2bb55081434054414 to fc29c1de46013777a5e4e2b592b9e0e0cf3e21d0

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

496102dsrc/sage/symbolic/ginac/utils.h, pynac-config.h: Eliminate use of PYNAC_SIZEOF_LONG, PYNAC_SIZEOF_LONG_LONG
fc29c1dsrc/sage/symbolic/pynac.pyx, pynac.pxd: Fixup distutils: sources

comment:11 Changed 4 months ago by git

  • Commit changed from fc29c1de46013777a5e4e2b592b9e0e0cf3e21d0 to add5e891706dd779b6129cea0e8dc175c3c48054

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

8223127src/sage/symbolic/ginac/utils.h: Fixup
add5e89src/sage/symbolic/ginac/utils.cpp: Fixup removal of version.h

comment:12 Changed 4 months ago by mkoeppe

  • Status changed from new to needs_review

comment:13 Changed 4 months ago by mkoeppe

  • Description modified (diff)

comment:14 Changed 4 months ago by mkoeppe

This mostly works but I'm getting a few timeouts/crashes that I'm not sure about

sage -t --random-seed=0 src/sage/schemes/elliptic_curves/ell_number_field.py  # Timed out
sage -t --random-seed=0 src/sage/manifolds/differentiable/degenerate_submanifold.py  # 1 doctest failed
sage -t --random-seed=0 src/sage/manifolds/differentiable/tensorfield.py  # Timed out
sage -t --random-seed=0 src/sage/plot/plot3d/parametric_plot3d.py  # Timed out
sage -t --random-seed=0 src/sage/plot/plot3d/plot3d.py  # Timed out
sage -t --random-seed=0 src/sage/plot/plot3d/base.pyx  # Timed out
sage -t --random-seed=0 src/sage/plot/plot3d/implicit_plot3d.py  # Timed out
sage -t --random-seed=0 src/sage/symbolic/relation.py  # Timed out
sage -t --random-seed=0 src/sage/symbolic/expression.pyx  # Killed due to illegal instruction
sage -t --random-seed=0 src/sage/symbolic/pynac_constant.pyx  # 18 doctests failed
Last edited 4 months ago by mkoeppe (previous) (diff)

comment:15 Changed 4 months ago by git

  • Commit changed from add5e891706dd779b6129cea0e8dc175c3c48054 to d9a14207300cc7baece7289cf309e7db1bfdebe1

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

d9a1420src/sage/symbolic/pynac_constant.pyx: Fix imports in doctests

comment:16 Changed 4 months ago by mkoeppe

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 4 months ago by mkoeppe

Oh, this is #28357 and I forgot to apply realpartloop.patch

comment:18 Changed 4 months ago by git

  • Commit changed from d9a14207300cc7baece7289cf309e7db1bfdebe1 to bc0d5f379151013fdcfd942fc0fea030feb11456

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

bc0d5f3Trac #28357: patch power::real_part

comment:19 follow-up: Changed 4 months ago by mkoeppe

Ready for testing and review. (I think the patchbot will not run because build/pkgs/pynac/ is removed by the ticket.)

comment:20 Changed 4 months ago by git

  • 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 4 months ago by fbissey

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 4 months ago by mkoeppe

I can of course move the pynac removal to a follow-up ticket

comment:23 Changed 4 months ago by git

  • 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
c8c3f53Trac #28357: patch power::real_part
e117724Move stuff from sage.libs.pynac to sage.symbolic.ginac, leaving some deprecated reimports behind
e166523src/sage/symbolic/ginac/version.h.in: Remove
3e428desrc/sage/symbolic/ginac/pynac-config.h: New
dec1c40src/sage/symbolic/ginac/utils.h, pynac-config.h: Eliminate use of PYNAC_SIZEOF_LONG, PYNAC_SIZEOF_LONG_LONG
17d6d2dsrc/sage/symbolic/pynac_constant.pyx: Fix imports in doctests

comment:24 Changed 4 months ago by mkoeppe

  • Description modified (diff)

comment:25 Changed 4 months ago by mkoeppe

  • 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/sage-9.1/local/lib/python3.7/site-packages/sage/symbolic/expression.cpython-37m-x86_64-linux-gnu.so: undefined symbol: _ZTIN5GiNaC5basicE

(as reported by a patchbot; also verified using tox -e docker-ubuntu-focal-standard)

comment:26 Changed 4 months ago by git

  • Commit changed from 17d6d2d5e8cb021481d4a4b1d483d924097e504b to 3355234a8d74da9413d18c44dd987897d05ed0fe

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

9e17a4bsrc/sage/symbolic/pynac_wrap.h: Clean up #include
3355234Replace 'from sage.symbolic.pynac cimport *' by more specific cimports

comment:27 follow-up: Changed 4 months ago by mkoeppe

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 4 months ago by gh-kliem

I will take a look, if I can resolve this.

comment:29 Changed 4 months ago by gh-kliem

  • Branch changed from u/mkoeppe/merge_pynac_as_src_sage_symbolic_ginac to u/gh-kliem/merge_pynac_as_src_sage_symbolic_ginac
  • Commit changed from 3355234a8d74da9413d18c44dd987897d05ed0fe to 04397d2e547f0a81c727165d9e9fee0d54b3ee6d

New commits:

01dfef3missing headers
04397d2add missing singular libraries

comment:30 Changed 4 months ago by git

  • Commit changed from 04397d2e547f0a81c727165d9e9fee0d54b3ee6d to 70bcddf168950f477c2b56fca725e668a1236fb6

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

70bcddfsimpler inclusion of headers

comment:31 Changed 4 months ago by git

  • Commit changed from 70bcddf168950f477c2b56fca725e668a1236fb6 to 7ce050fdea338610b91680584c0db59e54d4c5cf

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

7ce050fsomewhat fix doctest

comment:32 Changed 4 months ago by gh-kliem

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/site-packages/sage/symbolic/pynac.cpython-37m-x86_64-linux-gnu.so
    12"""
    23Symbolic 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 follow-up: Changed 4 months ago by gh-kliem

Once more thing: The headers in pkgs/sagemath-standard/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
01dfef3missing headers

comment:34 follow-up: Changed 4 months ago by 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

comment:35 Changed 4 months ago by mkoeppe

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 4 months ago by mkoeppe

Replying to gh-kliem:

Once more thing: The headers in pkgs/sagemath-standard/build/cythonized/ are not cleaned up.

Do we already have a ticket for this? If not, please open one

However, I think we should move away from the custom build system of Sage. I have opened #32390 for the next step in this.

comment:37 in reply to: ↑ 27 ; follow-up: Changed 4 months ago by mkoeppe

  • Dependencies set to #32391

Replying to mkoeppe:

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)

For cleaning up ring.pyx, I have opened #32391.

comment:38 in reply to: ↑ 37 ; follow-up: Changed 4 months ago by gh-kliem

Replying to mkoeppe:

Replying to mkoeppe:

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)

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 --enable-editable and will report whether this resolves anything.

comment:39 in reply to: ↑ 34 Changed 4 months ago by gh-kliem

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 4 months ago by mkoeppe

Replying to gh-kliem:

Replying to mkoeppe:

Replying to mkoeppe:

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)

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 4 months ago by gh-kliem

Ok, same problem with ./configure --enable-editable.

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 4 months ago by mkoeppe

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 4 months ago by gh-kliem

Using pxi is outdated, I was told. But maybe using something outdated is more desirable than having huge files.

comment:44 Changed 4 months ago by mkoeppe

That's probably just from the warning at https://cython.readthedocs.io/en/latest/src/userguide/language_basics.html?highlight=include#the-include-statement-and-include-files; 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 4 months ago by mkoeppe

  • Branch changed from u/gh-kliem/merge_pynac_as_src_sage_symbolic_ginac to u/mkoeppe/merge_pynac_as_src_sage_symbolic_ginac

comment:46 Changed 4 months ago by mkoeppe

  • Commit changed from 7ce050fdea338610b91680584c0db59e54d4c5cf to 188714b3bb4f953e1fb56c2841899e1e946d5900

It would look like this (currently does not compile because I haven't done the changes to ring.pyx)


New commits:

1f76b22missing headers
ff21838add missing singular libraries
188714bsage.symbolic: Change extension modules cimporting from pynac to pxi files included in sage.symbolic.pynac

comment:47 Changed 4 months ago by mkoeppe

(cherry-picked some of your changes onto this branch)

comment:48 Changed 4 months ago by git

  • Commit changed from 188714b3bb4f953e1fb56c2841899e1e946d5900 to 57ff18ac158d3c2541332652314aab9cdfa9d900

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

57ff18aMake sage.symbolic.expression the extension module that includes pynac

comment:49 Changed 4 months ago by mkoeppe

or perhaps better like this.

comment:50 Changed 4 months ago by git

  • Commit changed from 57ff18ac158d3c2541332652314aab9cdfa9d900 to 6a2ada278e567b7fafd06ce97cad895e1f323f95

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

6a2ada2src/sage/symbolic/expression.pyx: Move 'distutils: sources' here

comment:51 follow-up: Changed 4 months ago by tscrim

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 bug-fix), but doesn't this make Sage more monolithic?

comment:52 in reply to: ↑ 51 Changed 4 months ago by mkoeppe

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 bug-fix), 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 sagemath-symbolics, 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 follow-up: Changed 4 months ago by tscrim

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 stop-gap until it comes time to split everything?

comment:54 in reply to: ↑ 53 Changed 4 months ago by mkoeppe

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 stop-gap 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 sub-distributions defined using one of several possible mechanisms. Take a look at sagemath-objects from #29865 (https://pypi.org/project/sagemath-objects/), for example: It defines a sub-distribution 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 4 months ago by mkoeppe

  • Dependencies changed from #32391 to #32391, #32407

comment:56 Changed 4 months ago by git

  • Commit changed from 6a2ada278e567b7fafd06ce97cad895e1f323f95 to 759aa5d012d2ee2924456cb8df33514e351d2ffb

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

ef22257src/sage/symbolic/function.pyx: Undo rename
4ac54dfsrc/sage/symbolic/function.pyx: Replace cimport * by more specific cimport
6f49aeesage.symbolic.function.Function.__call__: Refactor through new helper function sage.symbolic.expression.call_by_ginac_serial
2f92b90sage.symbolic.function.Function._register_function: Refactor through new function sage.symbolic.expression.register_or_update_function
e81e6c7sage.symbolic.function.GinacFunction._register_function: Refactor through sage.symbolic.expression.register_or_update_function
d397ef1sage.symbolic.function: Remove unused imports, cimports
1c43dbesage.symbolic.function.{GinacFunction,BuiltinFunction}._is_registered: Refactor through new function sage.symbolic.expression.find_registered_function; remove last pynac import
9d4fe5fsage.symbolic.function: Remove unused cimport
9a9fdc7Merge #32407
759aa5dsage.symbolic.expression: Finish merging code using pynac into this one DSO

comment:57 Changed 4 months ago by mkoeppe

It remains to remove the import cycle sage.symbolic.ring -> sage.symbolic.expression -> sage.symbolic.function -> sage.symbolic.ring

comment:58 Changed 4 months ago by tscrim

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 4 months ago by git

  • Commit changed from 759aa5d012d2ee2924456cb8df33514e351d2ffb to 46c4a6b0b6815076d8f0f5925459bb221f2c408b

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

b640014Replace import from sage.symbolic.pynac by sage.symbolic.expression
46c4a6bsage.symbolic.function: Move sfunction_serial_dict, get_sfunction_from_serial to .expression, refactor

comment:60 Changed 4 months ago by git

  • Commit changed from 46c4a6b0b6815076d8f0f5925459bb221f2c408b to 903792a901e783b70244efa2f8ed6cb4f1eaf454

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

903792asrc/sage/symbolic/pynac_constant.py: New

comment:61 Changed 4 months ago by mkoeppe

  • Description modified (diff)

comment:62 Changed 4 months ago by git

  • Commit changed from 903792a901e783b70244efa2f8ed6cb4f1eaf454 to 9f56aa26d702555868d3ba6d9b9338fc19aac390

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

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

comment:63 Changed 4 months ago by git

  • Commit changed from 9f56aa26d702555868d3ba6d9b9338fc19aac390 to 3153781f5e6c02b9a1a0cef9f8295f206a702162

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

3153781WIP: Try to break import cycle

comment:64 Changed 4 months ago by mkoeppe

Still struggling with an import cycle sage.symbolic.ring -> sage.symbolic.expression -> sage.symbolic.ring

comment:65 Changed 4 months ago by mkoeppe

Help welcome

comment:66 Changed 4 months ago by dimpase

I recall that i, a.k.a. I, is really close the the center of the import troubles.

comment:67 Changed 4 months ago by gh-kliem

  • Authors changed from Matthias Koeppe to Matthias Koeppe, Jonathan Kliem
  • Branch changed from u/mkoeppe/merge_pynac_as_src_sage_symbolic_ginac to u/gh-kliem/merge_pynac_as_src_sage_symbolic_ginac
  • Commit changed from 3153781f5e6c02b9a1a0cef9f8295f206a702162 to 7ce050fdea338610b91680584c0db59e54d4c5cf

New commits:

01dfef3missing headers
04397d2add missing singular libraries
70bcddfsimpler inclusion of headers
7ce050fsomewhat fix doctest

comment:68 Changed 4 months ago by git

  • Commit changed from 7ce050fdea338610b91680584c0db59e54d4c5cf to 48267a7cc19287bd9523370ea9ebbda6d3f1085b

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

9a9fdc7Merge #32407
759aa5dsage.symbolic.expression: Finish merging code using pynac into this one DSO
b640014Replace import from sage.symbolic.pynac by sage.symbolic.expression
46c4a6bsage.symbolic.function: Move sfunction_serial_dict, get_sfunction_from_serial to .expression, refactor
903792asrc/sage/symbolic/pynac_constant.py: New
9f56aa2sage.symbolic.ring: cimport the helper functions from sage.symbolic.expression (safe because expression.pxd no longer declares the Expression class)
3153781WIP: Try to break import cycle
a56c69dfix startup
5b557c6deprecation warnings
48267a7fix doctests

comment:69 Changed 4 months ago by gh-kliem

  • Status changed from needs_work to needs_review

Seems to work now.

comment:70 Changed 4 months ago by git

  • Commit changed from 48267a7cc19287bd9523370ea9ebbda6d3f1085b to 5776163f25a6a26bb0063b56ad01517907c00b3a

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

5776163wrong import

comment:71 Changed 4 months ago by mkoeppe

Cool, thanks very much!!

comment:72 follow-up: Changed 4 months ago by mkoeppe

I'm getting

sage -t --random-seed=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 follow-ups: Changed 4 months ago by mkoeppe

  • 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

Last edited 4 months ago by mkoeppe (previous) (diff)

comment:74 Changed 4 months ago by mkoeppe

Also I don't think we need src/sage/symbolic/pynac.py -- the module sage.symbolic.pynac only had a short-lived existence here in this branch. Did you mean to add these lazy_import to src/sage/libs/pynac/pynac.py?

comment:75 Changed 4 months ago by mkoeppe

  • Branch changed from u/gh-kliem/merge_pynac_as_src_sage_symbolic_ginac to u/mkoeppe/merge_pynac_as_src_sage_symbolic_ginac

comment:76 Changed 4 months ago by gh-kliem

  • Branch changed from u/mkoeppe/merge_pynac_as_src_sage_symbolic_ginac to u/gh-kliem/merge_pynac_as_src_sage_symbolic_ginac
  • Commit changed from 5776163f25a6a26bb0063b56ad01517907c00b3a to a27d820bb00ddcd82353eec55eab8920920068b7

New commits:

c12e71emoved imports to libs/pynac/pynac.py
b898086fix serial getting of BuiltinFunction
7e17150fixed import in doctest
4ec43a2src/sage/symbolic/ginac/README.md: New
a27d820Merge branch 'u/mkoeppe/merge_pynac_as_src_sage_symbolic_ginac' of git://trac.sagemath.org/sage into u/gh-kliem/merge_pynac_as_src_sage_symbolic_ginac

comment:77 in reply to: ↑ 72 Changed 4 months ago by gh-kliem

Replying to mkoeppe:

I'm getting

sage -t --random-seed=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 4 months ago by mkoeppe

  • Branch changed from u/gh-kliem/merge_pynac_as_src_sage_symbolic_ginac to u/mkoeppe/merge_pynac_as_src_sage_symbolic_ginac

comment:79 Changed 4 months ago by git

  • Commit changed from a27d820bb00ddcd82353eec55eab8920920068b7 to 2d2153446fc3bb4d2d17d32cc770c0efc5121ecf

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

2d21534src/sage/symbolic/expression.pyx: Update copyright according to 'git blame -w --date=format:%Y src/sage/symbolic/expression.pyx | sort -k2'

comment:80 Changed 4 months ago by mkoeppe

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 4 months ago by gh-kliem

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:

https://git.sagemath.org/sage.git/commit/?h=2d2153446fc3bb4d2d17d32cc770c0efc5121ecf&id=5b557c68dd722ae8063334b23a595f9b1482a5d7

comment:82 Changed 4 months ago by git

  • Commit changed from 2d2153446fc3bb4d2d17d32cc770c0efc5121ecf to 03a4b83be3c6379679cb1b138ee440f60db2710c

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

03a4b83src/sage/symbolic/function.pyx: Update copyright according to 'git blame -w --date=format:%Y src/sage/symbolic/function.pyx | sort -k2'

comment:83 Changed 4 months ago by git

  • Commit changed from 03a4b83be3c6379679cb1b138ee440f60db2710c to 53453b4d897219e536450651225fb2d2a0b71e92

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

53453b4src/sage/symbolic/*_impl.pxi: Update copyright according to git blame

comment:84 in reply to: ↑ 73 Changed 4 months ago by mkoeppe

  • 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 4 months ago by mkoeppe

I cannot reproduce these timings any more; perhaps this was just from CPU throttling.

comment:86 Changed 4 months ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:87 Changed 4 months ago by gh-kliem

  • Branch changed from u/mkoeppe/merge_pynac_as_src_sage_symbolic_ginac to u/gh-kliem/merge_pynac_as_src_sage_symbolic_ginac
  • Commit changed from 53453b4d897219e536450651225fb2d2a0b71e92 to e79cc47188db3896891d951f63973994d64251be

New commits:

e79cc47sporadic failing tests

comment:88 Changed 4 months ago by mkoeppe

  • Branch changed from u/gh-kliem/merge_pynac_as_src_sage_symbolic_ginac to u/mkoeppe/merge_pynac_as_src_sage_symbolic_ginac

comment:89 Changed 4 months ago by git

  • Commit changed from e79cc47188db3896891d951f63973994d64251be to ac9c9594cc1d0e12cb91214a9be3d8c1ec293e4d

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

ac9c959sage.symbolic.pynac_constant: Use lazy_import with deprecation

comment:90 Changed 4 months ago by git

  • Commit changed from ac9c9594cc1d0e12cb91214a9be3d8c1ec293e4d to e9c8637392c1a811b53b4d9ae21799d02b38a8c8

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

d680c35sage.symbolic.expression.call_registered_function: Rename from call_by_ginac_serial
1f761d8src/sage/symbolic/pynac_function_impl.pxi: Split out from src/sage/symbolic/expression.pyx
47adbccsrc/sage/symbolic/pynac_function_impl.pxi: Add doctests
1d07a02Merge #32407
e9c8637get_sfunction_from_hash: Add doctest

comment:91 follow-up: Changed 4 months ago by 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)

comment:92 in reply to: ↑ 91 Changed 4 months ago by fbissey

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 3 months ago by mkoeppe

sage -t --long --random-seed=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 3 months ago by gh-kliem

  • Branch changed from u/mkoeppe/merge_pynac_as_src_sage_symbolic_ginac to u/gh-kliem/merge_pynac_as_src_sage_symbolic_ginac
  • Commit changed from e9c8637392c1a811b53b4d9ae21799d02b38a8c8 to 712f9261c25496053ae203ec6b12b6410560f01c

Fixed that test in sageinspect.py.


New commits:

712f926fix doctest getting source code

comment:95 Changed 3 months ago by git

  • Commit changed from 712f9261c25496053ae203ec6b12b6410560f01c to 5d062ed36ce07dde3c9ba0b3ae7e39e825a6a1b2

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

97060e9fix docbuild
5d062edone more doctest

comment:96 Changed 3 months ago by gh-kliem

Coverage is still an issue, e.g. new_Expression_from_pyobject doesn't even have a docstring.

comment:97 Changed 3 months ago by mkoeppe

  • Branch changed from u/gh-kliem/merge_pynac_as_src_sage_symbolic_ginac to u/mkoeppe/merge_pynac_as_src_sage_symbolic_ginac

comment:98 Changed 3 months ago by mkoeppe

  • Commit changed from 5d062ed36ce07dde3c9ba0b3ae7e39e825a6a1b2 to 3f1ac2e8a415de7612f3b9c5ad82165761b61fad

I've solved this by consolidating two similar functions into one


New commits:

3f1ac2esage.symbolic.expression: Merge new_Expression_from_pyobject and new_Expression_force_pyobject

comment:99 Changed 3 months ago by mkoeppe

  • Reviewers set to https://github.com/mkoeppe/sage/actions/runs/1168763794, https://github.com/mkoeppe/sage/actions/runs/1168763799, ...

comment:100 Changed 3 months ago by git

  • Commit changed from 3f1ac2e8a415de7612f3b9c5ad82165761b61fad to 0643756930700a165b83987dc9d51e6105e2945a

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

0643756new_Expression_from_pyobject: Add output for new doctests

comment:101 Changed 3 months ago by git

  • Commit changed from 0643756930700a165b83987dc9d51e6105e2945a to 327ca5f3f5ca2c28d0c66685a2b528c88c10d265

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

7f1e5besrc/sage/symbolic/pynac_function_impl.pxi: Remove unused import in doctest
06e8c31src/sage/symbolic/pynac_function_impl.pxi: Fix doc markup
327ca5fMerge #32407

comment:102 Changed 3 months ago by mkoeppe

The portability tests on GH Actions ran through successfully.

Ready for review

comment:103 Changed 3 months ago by mkoeppe

  • Cc nbruin added

comment:104 Changed 3 months ago by dimpase

there is a failing test:

e023915d474 (Markus Wageringel   2021-06-15 19:54:54 +0200 340)     sage: result = integral(sinh(x^2+sqrt(x-1)),x)  # long time (15s on sage.math, 2012)
e023915d474 (Markus Wageringel   2021-06-15 19:54:54 +0200 341)     ...
e023915d474 (Markus Wageringel   2021-06-15 19:54:54 +0200 342)     sage: result
d0c52e4aa36 (Jean-Pierre Flori   2013-05-27 15:36:24 +0200 343)     integrate(sinh(x^2 + sqrt(x - 1)), x)

line 342 needs # long time tag.

comment:105 Changed 3 months ago by mkoeppe

Unrelated to this ticket but yes

comment:106 Changed 3 months ago by mkoeppe

opened #32461 for this

comment:107 Changed 3 months ago by dimpase

  • 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 3 months ago by mkoeppe

Thank you!

comment:109 Changed 3 months ago by dimpase

should #32461 be a dependency here?

comment:110 Changed 3 months ago by mkoeppe

  • Dependencies changed from #32391, #32407 to #32391, #32407, #32461

comment:111 Changed 3 months ago by git

  • 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:

0d0b58fMerge tag '9.5.beta1' into t/32386/merge_pynac_as_src_sage_symbolic_ginac

comment:112 Changed 3 months ago by mkoeppe

  • Status changed from needs_review to positive_review

comment:113 Changed 3 months ago by vbraun

  • 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 2 months ago by egourgoulhon

  • 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 i7-8665U laptop:

  • Sage 9.4: 461 s
  • Sage 9.5.beta2: 449 s

comment:115 Changed 2 months ago by mkoeppe

Thanks for testing!

Note: See TracTickets for help on using tickets.