sage.symbolic.function: Remove direct use of cimports from pynac

Question

sage.symbolic.function: Remove direct use of cimports from pynac

Closed this issue 4 years ago · 34 comments

Similar to what is done in #32391. Mechanical refactoring to use new helper functions defined in sage.symbolic.expression instead of directly cimporting from sage.libs.pynac

Preparation for #32386.

CC: @kliem @tscrim

Component: symbolics

Author: Matthias Koeppe

Branch/Commit: 06e8c31

Reviewer: Jonathan Kliem

Issue created by migration from https://trac.sagemath.org/ticket/32407

Answer 1 · 2021-08-22T04:17:10.000Z

Branch: u/mkoeppe/sage_symbolic_function__remove_direct_use_of_cimports_from_pynac

Answer 2 · 2021-08-22T20:20:18.000Z

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

`50048aa`	`sage.symbolic.function.Function._register_function: Refactor through new function sage.symbolic.expression.register_or_update_function`
`a2f97a1`	`sage.symbolic.function.GinacFunction._register_function: Refactor through sage.symbolic.expression.register_or_update_function`

Answer 3 · 2021-08-22T20:20:18.000Z

Commit: a2f97a1

Answer 4 · 2021-08-22T20:25:05.000Z

Changed commit from a2f97a1 to e81e6c7

Answer 5 · 2021-08-22T20:25:05.000Z

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

`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`

Answer 6 · 2021-08-22T21:28:41.000Z

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

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

Answer 7 · 2021-08-22T21:28:41.000Z

Changed commit from e81e6c7 to d397ef1

Answer 8 · 2021-08-22T21:49:49.000Z

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

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

Answer 9 · 2021-08-22T21:49:49.000Z

Changed commit from d397ef1 to 1c43dbe

Answer 10 · 2021-08-22T21:52:10.000Z

Changed dependencies from #32391 to none

Answer 11 · 2021-08-22T21:52:10.000Z

Author: Matthias Koeppe

Answer 12 · 2021-08-22T21:52:10.000Z

Description changed:

--- 
+++ 
@@ -1,4 +1,4 @@
-Similar and in extension of #32391.
+Similar to what is done in #32391. Mechanical refactoring to use new helper functions defined in `sage.symbolic.expression` instead of directly `cimport`ing from `sage.libs.pynac`
 
 Preparation for #32386.

Answer 13 · 2021-08-22T21:53:17.000Z

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

`9d4fe5f`	`sage.symbolic.function: Remove unused cimport`

Answer 14 · 2021-08-22T21:53:17.000Z

Changed commit from 1c43dbe to 9d4fe5f

Answer 15 · 2021-08-23T06:05:40.000Z

Reviewer: Jonathan Kliem

Answer 16 · 2021-08-23T06:05:40.000Z

comment:8

The cpdef functions should have doctests for coverage.

Answer 17 · 2021-08-24T18:51:03.000Z

Changed commit from 9d4fe5f to d680c35

Answer 18 · 2021-08-24T18:51:03.000Z

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

`d680c35`	`sage.symbolic.expression.call_registered_function: Rename from call_by_ginac_serial`

Answer 19 · 2021-08-24T20:39:01.000Z

Changed commit from d680c35 to 47adbcc

Answer 20 · 2021-08-24T20:39:01.000Z

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

`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`

Answer 21 · 2021-08-25T07:26:25.000Z

comment:12

         sage: from sage.symbolic.function import BuiltinFunction
-        sage: from sage.symbolic.expression import register_or_update_function, call_registered_function
         sage: class Archosaurian(BuiltinFunction):
         ....:     def __init__(self):
         ....:         BuiltinFunction.__init__(self, 'archsaur', nargs=1)
         ....:     def _eval_(self, x):
         ....:         return x * exp(x)
         sage: archsaur = Archosaurian()  # indirect doctest
         sage: archsaur(2)
         2*e^2

The import isn't used.

Answer 22 · 2021-08-25T21:51:39.000Z

Changed commit from 47adbcc to 7f1e5be

Answer 23 · 2021-08-25T21:51:39.000Z

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

`7f1e5be`	`src/sage/symbolic/pynac_function_impl.pxi: Remove unused import in doctest`

Answer 24 · 2021-08-26T18:31:25.000Z

comment:15

patchbot is morally green (failures are from a missing docbuild on the patchbot)

Answer 25 · 2021-08-26T19:18:29.000Z

comment:16

Actually this ticket leads to failure of docbuild and this should be fixed.

It is fixed in #32386 and can be somewhat cherry-picked from there.

The problem seems to be with

:class:`Expression`s

This doesn't seem to work, but you can do

:class:`Expression` (Expressions)

which supposedly has Expressions as the link text
(for some reason I have fixed it with Expression's in #32386, which is wrong grammar.

Answer 26 · 2021-08-26T19:21:42.000Z

comment:17

Ah! Thanks for the explanation

Answer 27 · 2021-08-26T19:36:16.000Z

Changed commit from 7f1e5be to 06e8c31

Answer 28 · 2021-08-26T19:36:16.000Z

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

`06e8c31`	`src/sage/symbolic/pynac_function_impl.pxi: Fix doc markup`

Answer 29 · 2021-08-26T23:47:46.000Z

comment:19

Replying to @kliem:

Actually this ticket leads to failure of docbuild and this should be fixed.

It is fixed in #32386 and can be somewhat cherry-picked from there.

The problem seems to be with
:class:`Expression`s
This doesn't seem to work, but you can do
:class:`Expression` (Expressions)
which supposedly has Expressions as the link text
(for some reason I have fixed it with Expression's in #32386, which is wrong grammar.

That is also not quite correct as I believe you want

:class:`Expressions <Expression>`

to show Expressions but link to the class Expression. Although you can also do something like :class:`Expression`-s or 's, but I won't worry too much about the grammar there...

Answer 30 · 2021-08-27T00:02:11.000Z

comment:20

In the meantime (in 06e8c31) I have reworded it in a different way

Answer 31 · 2021-08-27T06:09:41.000Z

comment:21

Replying to @tscrim:

Replying to @kliem:
Actually this ticket leads to failure of docbuild and this should be fixed.

It is fixed in #32386 and can be somewhat cherry-picked from there.

The problem seems to be with
:class:`Expression`s
This doesn't seem to work, but you can do
:class:`Expression` (Expressions)
which supposedly has Expressions as the link text
(for some reason I have fixed it with Expression's in #32386, which is wrong grammar.
That is also not quite correct as I believe you want
:class:`Expressions <Expression>`
to show Expressions but link to the class Expression. Although you can also do something like :class:`Expression`-s or 's, but I won't worry too much about the grammar there...

I see. I got confused by the documentation.

Answer 32 · 2021-08-27T16:13:53.000Z

comment:22

LGTM.

Answer 33 · 2021-08-27T18:47:49.000Z

comment:23

Thank you!

Answer 34 · 2021-09-13T22:19:05.000Z

Changed branch from u/mkoeppe/sage_symbolic_function__remove_direct_use_of_cimports_from_pynac to 06e8c31