Deprecate sage.interfaces is_...Element functions
Closed this issue · 48 comments
Introducing a module sage.interfaces.abc with abstract base classes for isinstance testing (see https://doc.sagemath.org/html/en/developer/packaging_sage_library.html#module-level-runtime-dependencies)
Part of #32414
Depends on #34770
Depends on #34823
Component: refactoring
Author: Matthias Koeppe, Dima Pasechnik
Branch/Commit: b9eac04
Reviewer: Dima Pasechnik
Issue created by migration from https://trac.sagemath.org/ticket/34804
Description changed:
---
+++
@@ -1,2 +1,3 @@
Introducing a module `sage.interfaces.abc` with abstract base classes for `isinstance` testing (see https://doc.sagemath.org/html/en/developer/packaging_sage_library.html#module-level-runtime-dependencies)
+Part of #32414--- a/src/sage/interfaces/gap.py
+++ b/src/sage/interfaces/gap.py
@@ -211,6 +211,9 @@ import platform
import string
import warnings
+import sage.rings.abc
+
+
WORKSPACE = gap_workspace_file()
first_try = True
@@ -1524,7 +1527,7 @@ def gap_reset_workspace(max_workspace_size=None, verbose=False):
@instancedoc
-class GapElement(GapElement_generic):
+class GapElement(GapElement_generic, sage.rings.abc.GapElement):
def __getitem__(self, n):
"""
EXAMPLES::this is weird - do you mean sage.interfaces... there, not sage.rings... ?
New commits:
6031878 | sage.interfaces.abc.GapElement: New, use it instead of sage.interfaces.gap.is_GapElement |
Yes, sorry, it should be sage.interfaces... of course
I took (corrected) pieces of this branch to #34770. So this ought to be based on the branch there.
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
21e38a7 | fix coersion of libgap FFelements; switch to libgap |
d4bde80 | make imports of is_GapEelment uniform across files |
eb72ef0 | avoid is_GapElement |
1c4a69f | use GapElement from an abstract superclass |
6e5ea79 | move import of libgap into a function |
8572f15 | sage.interfaces.abc.GapElement: New, use it instead of sage.interfaces.gap.is_GapElement |
Branch pushed to git repo; I updated commit sha1. New commits:
adc231e | remove is_GapElement from number fields code |
7c67a10 | deprecating is_GapElement |
6c75e9b | better rst, point at sage.interfaces.abc to use |
e99db3a | src/sage/interfaces/gap.py: Reviewer edits |
b802415 | Merge #34823 |
27c8788 | src/sage/interfaces/abc.py (GpElement): New, use it for isinstance testing |
Author: Matthias Koeppe, ...
Branch pushed to git repo; I updated commit sha1. New commits:
106a85b | src/sage/interfaces/abc.py (SingularElement, Macaulay2Element): New, use for isinstance testing |
Changed author from Matthias Koeppe, ... to Matthias Koeppe
Branch pushed to git repo; I updated commit sha1. New commits:
2604174 | src/sage/interfaces/abc.py: Add doc |
Reviewer: Dima Pasechnik
we have still only a fraction done:
Axiom ?
Expect ?
FriCAS ?
Gap done
Gp done
Interface ?
Kash ?
LiE ?
Lisp ?
Macaulay2 done
Magma ?
Maxima ?
MaximaLib ?
R ?
Singular done
How about an implementation with a decorator:
r"""
Abstract base classes for interface elements
"""
def _add_doc():
def wrapped(c):
c.__doc__ = "\n Abstract base class for :class:`~sage.interfaces.gap." + c.__name__
c.__doc__ += """
This class is defined for the purpose of ``isinstance`` tests. It should not be
instantiated.
EXAMPLES:
By design, there is a unique direct subclass::
"""
c.__doc__ += " sage: len(sage.interfaces.abc."+c.__name__+".__subclasses__()) <= 1\n True"
return c
return wrapped
@_add_doc()
class GapElement: pass
@_add_doc()
class GpElement: pass
...Needless to say, docstrings etc work as they should this way. Decorator overhead - very small, if at all, IMHO.
Doctests are discovered by parsing files; they cannot be supplied by decorators.
Changed author from Matthias Koeppe to Matthias Koeppe, Dima Pasechnik
Branch pushed to git repo; I updated commit sha1. New commits:
31128ee | sage.interfaces.abc: Add MagmaElement, deprecate is_MagmaElement |
The remaining is_... functions are not used anywhere
Branch pushed to git repo; I updated commit sha1. New commits:
56b8bf6 | sage.interfaces.abc: Add AxiomElement, deprecate is_AxiomElement |
Branch pushed to git repo; I updated commit sha1. New commits:
e957fc9 | sage.interfaces.abc: Add FriCASElement, deprecate is_FriCASElement |
Branch pushed to git repo; I updated commit sha1. New commits:
b9eac04 | sage.interface: Deprecate unused is_*Element functions, but do not introduce ABCs for them |
Green checks, ready for review
Replying to Matthias Köppe:
Doctests are discovered by parsing files; they cannot be supplied by decorators.
Yes, they can. In fact, very easy to amend and doctest the docstring of a function:
$ cat d.py
def docstr(f):
"""
amends the docstring
"""
f.__doc__ += """
>>> tst(1)
42
"""
return f
@docstr
def tst(x):
'''adds 1 to x
>>> tst(0)
1
>>> tst(42)
42
'''
return x+1
if __name__ == '__main__':
import doctest
doctest.testmod()
$
$ python3 d.py
**********************************************************************
File "/tmp/d.py", line 16, in __main__.tst
Failed example:
tst(42)
Expected:
42
Got:
43
**********************************************************************
File "/tmp/d.py", line 19, in __main__.tst
Failed example:
tst(1)
Expected:
42
Got:
2
**********************************************************************
1 items had failures:
2 of 3 in __main__.tst
See, the original tst() had 2 doctests, one was added by @docstr - and 3 were tested.
So it's perfectly doable for functions.
As a sanity check:
$ ipython3
Python 3.9.2 (default, Feb 28 2021, 17:03:44)
Type 'copyright', 'credits' or 'license' for more information
IPython 7.20.0 -- An enhanced Interactive Python. Type '?' for help.
In [1]: from d import *
In [2]: tst?
Docstring:
adds 1 to x
>>> tst(0)
1
>>> tst(42)
42
>>> tst(1)
42
File: /tmp/d.py
Type: function
As we see, doctests are run on __doc__ attributes of functions, not by scraping the text of the file.
And it works, in vanilla Python 3.9, with classes too:
def docstr(f):
"""
amends the docstring
"""
f.__doc__ += """
>>> len(tst.__subclasses__()) > 0
True
"""
return f
@docstr
class tst:
'''testing
>>> len(tst.__subclasses__()) <= 1
True
'''
pass
if __name__ == '__main__':
import doctest
doctest.testmod()and so we see 2 doctests being run, one from the class body, one added by the decorator.
$ python3 c.py
**********************************************************************
File "/tmp/c.py", line 17, in __main__.tst
Failed example:
len(tst.__subclasses__()) > 0
Expected:
True
Got:
False
**********************************************************************
1 items had failures:
1 of 2 in __main__.tst
***Test Failed*** 1 failures.
(You can easily modify this so that the class has empty __doc__, then one can create it in the decorator, so it's all sane here in vanilla Python).
I can only conclude that Sage's doctest system is borked at this point,and somehow
does a non-vanilla thing.
Indeed, sage -t c.py does not see the doctests added by the decorator.
I've opened #34828 to fix the doctest issue in comment:28
Let's not wait for that to be fixed
lgtm
Thank you!
Changed branch from u/mkoeppe/deprecate_sage_interfaces_is____element_functions to b9eac04