-
-
Notifications
You must be signed in to change notification settings - Fork 488
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merge pynac sources as src/sage/symbolic/ginac #32386
Comments
This comment has been minimized.
This comment has been minimized.
comment:3
I ran Last 10 new commits:
|
Commit: |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:5
Next: Move stuff from Help from Cython experts very welcome... |
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:8
This version compiles OK |
Author: Matthias Koeppe |
This comment has been minimized.
This comment has been minimized.
comment:14
This mostly works but I'm getting a few timeouts/crashes that I'm not sure about
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:16
The crash is from this doctest:
which seems to lead to an infinite recursion through |
comment:17
Oh, this is #28357 and I forgot to apply |
Changed reviewer from https://github.com/mkoeppe/sage/actions/runs/1168763794, https://github.com/mkoeppe/sage/actions/runs/1168763799, ... to Dima Pasechnik |
comment:107
lgtm |
comment:108
Thank you! |
comment:109
should #32461 be a dependency here? |
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
|
Changed branch from u/mkoeppe/merge_pynac_as_src_sage_symbolic_ginac to |
Changed commit from |
comment:114
Replying to @mkoeppe:
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:
|
comment:115
Thanks for testing! |
…h#18036, sagemath#29738, sagemath#32386, sagemath#32638, sagemath#32665, sagemath#34215 <!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes sagemath#1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [ ] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36304 Reported by: Matthias Köppe Reviewer(s): David Coudert
…h#18036, sagemath#29738, sagemath#32386, sagemath#32638, sagemath#32665, sagemath#34215 <!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes sagemath#1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [ ] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36304 Reported by: Matthias Köppe Reviewer(s): David Coudert
…2386 (2021) <!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [ ] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#37873 Reported by: Matthias Köppe Reviewer(s): Kwankyu Lee
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 insage.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.) areimport
helper functions provided bysage.symbolic.expression
instead of having tocimport
Pynac functions directly (see sage.symbolic.ring: Remove direct use of cimports from pynac #32391, sage.symbolic.function: Remove direct use of cimports from pynac #32407)sage.symbolic.expression
(via Cythoninclude
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
Depends on #32391
Depends on #32407
Depends on #32461
CC: @kliem @kiwifb @antonio-rojas @DaveWitteMorris @slel @dimpase @tscrim @egourgoulhon @nbruin
Component: symbolics
Author: Matthias Koeppe, Jonathan Kliem
Branch:
0d0b58f
Reviewer: Dima Pasechnik
Issue created by migration from https://trac.sagemath.org/ticket/32386
The text was updated successfully, but these errors were encountered: