sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.47k stars 487 forks source link

add type information to _repr_ of Macaulay2 elements #28304

Closed mwageringel closed 5 years ago

mwageringel commented 5 years ago

In Macaulay2, the AfterPrint is responsible for adding useful type information about computed results. Since this information generally cannot be obtained easily otherwise, it should be added to the _repr_ of Macaulay2 elements. Compare for example

sage: macaulay2.eval('kernel matrix {{1, 2}, {3, 6}}')
image | 2  |
      | -1 |

                          2
ZZ-module, submodule of ZZ

to

sage: macaulay2(matrix([[1, 2], [3, 6]])).kernel()
image | 2  |
      | -1 |
sage: _.cls()  # this is much less informative
Module

CC: @dimpase @antonleykin

Component: interfaces: optional

Keywords: macaulay2

Author: Markus Wageringel

Branch/Commit: 2324c95

Reviewer: Dima Pasechnik, Travis Scrimshaw

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

mwageringel commented 5 years ago
comment:1

With #28303, it would be easy to implement this as follows:

--- a/src/sage/interfaces/macaulay2.py
+++ b/src/sage/interfaces/macaulay2.py
@@ -802,9 +802,12 @@ class Macaulay2Element(ExtraTabCompletion, ExpectElement):
         """
         from sage.typeset.ascii_art import empty_ascii_art
         P = self.parent()
-        return P.eval('print(wrap(%d,"-",net %s))'
-                      % (empty_ascii_art._terminal_width(), self._name),
-                      strip=False)
+        # In M2, the wrapped output is indented by the width of the prompt,
+        # which we strip in Sage. We hardcode the width of the prompt to
+        # 14=len('o1000000001 = '), which is tested in the doctests by the
+        # output getting wrapped at 80 characters.
+        width = 14 + empty_ascii_art._terminal_width()
+        return P.eval('printWidth=%d;%s' % (width, self._name), strip=True)

     def external_string(self):
         """

This ticket would affect the repr of many Macaulay2 elements and consequently lots of doctests would need to be updated. I think this change is justified for its usefulness, but I welcome other ideas. Making this output optional would also be a possibility (QQbar has optional print options, too, for instance).

mwageringel commented 5 years ago

Author: Markus Wageringel

mwageringel commented 5 years ago

Commit: fd3ab69

mwageringel commented 5 years ago
comment:3

We discussed this issue at the IMA a few weeks ago. Dima suggested that including type information in the repr by default is not pythonic, but that Macaulay2 elements should have a method to access this kind of information instead. I think it would still be useful to be able to optionally enable printing this info, as this is what Macaulay2 does by default.

Here is a branch implementing both. It adds a method after_print_text() to obtain this type info from within Sage. Moreover, it adds an option macaulay2.options.after_print='yes' ('no' by default) to append the AfterPrint text to the repr of Macaulay2 elements. This option can then be enabled in the init.sage file, for example.

sage: m = macaulay2(matrix([[1, 2], [3, 6]])); m
| 1 2 |
| 3 6 |
sage: m.after_print_text()
         2        2
Matrix ZZ  <--- ZZ
sage: macaulay2.options.after_print = 'yes'
sage: m
| 1 2 |
| 3 6 |

         2        2
Matrix ZZ  <--- ZZ

New commits:

e25e16b28304: add after_print_text() for Macaulay2 elements
fd3ab6928304: add global options to Macaulay2 to enable AfterPrint
mwageringel commented 5 years ago

Branch: u/gh-mwageringel/28304

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from fd3ab69 to bcbf9aa

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

bcbf9aa28304: add global options to Macaulay2 to enable AfterPrint
dimpase commented 5 years ago

Reviewer: Dima Pasechnik

dimpase commented 5 years ago
comment:6

in docstrings, the 1st line should be followed by an empty line. So

+    def after_print_text(self):
+        r"""
+        Obtain the type information about this Macaulay2 element that is
+        displayed using ``AfterPrint`` in the Macaulay2 interpreter.

should become something like

+    def after_print_text(self):
+        r"""
+        Macaulay2 type information for ``self``
+
+        Obtain the type information for ``self``, as
+        displayed by ``AfterPrint`` in Macaulay2 interpreter.

Also, please document options class.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from bcbf9aa to 3fc7b99

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

3fc7b9928304: improve Macaulay2 documentation
mwageringel commented 5 years ago
comment:8

Thank you for the quick feedback. I have shortened the first sentence of the docstring, so that it fits on a single line, and moved the examples for the after_print option to the options class. The super class GlobalOptions actually generates the docstring if absent, but it is good to also have some examples there. (This is currently broken in HTML docs with Python 3 #28698.)

I have also moved the import of deprecated_function_alias to the top of the file, so that its docstring does not get added to the HTML documentation.

tscrim commented 5 years ago
comment:10

For the after_print option, IMO more natural values would be True and False rather than strings.

mwageringel commented 5 years ago
comment:11

Replying to @tscrim:

For the after_print option, IMO more natural values would be True and False rather than strings.

I completely agree with this. The problem is that sage.structure.global_options only seems to work with options of type string, but booleans do not work. I have not looked much into it yet, so cannot say how difficult it is to change this, but will report back once I know more.

tscrim commented 5 years ago
comment:12

Replying to @mwageringel:

Replying to @tscrim:

For the after_print option, IMO more natural values would be True and False rather than strings.

I completely agree with this. The problem is that sage.structure.global_options only seems to work with options of type string, but booleans do not work. I have not looked much into it yet, so cannot say how difficult it is to change this, but will report back once I know more.

That is wrong. See combinat/partition.py:

        diagram_str = dict(default="*",
                         description='The character used for the cells when printing Ferrers diagrams',
                         checker=lambda char: isinstance(char,str))

So use

        after_print = dict(default=True,
                           description='Print more stuff',
                           checker=lambda val: isinstance(val, bool))
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

2324c9528304: use booleans for after_print option
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 3fc7b99 to 2324c95

mwageringel commented 5 years ago
comment:14

Replying to @tscrim:

So use

        after_print = dict(default=True,
                           description='Print more stuff',
                           checker=lambda val: isinstance(val, bool))

Awesome. Thanks for pointing this out. I have changed it.

tscrim commented 5 years ago
comment:15

Dima, are you going to finish the review here or do you want me to do it?

dimpase commented 5 years ago
comment:16

Hi Travis, please go ahead with your review. I am a bit pressed for time now.

tscrim commented 5 years ago

Changed reviewer from Dima Pasechnik to Dima Pasechnik, Travis Scrimshaw

tscrim commented 5 years ago
comment:17

Okay, LGTM.

vbraun commented 5 years ago

Changed branch from u/gh-mwageringel/28304 to 2324c95