Opened 3 years ago
Closed 3 years ago
#28304 closed enhancement (fixed)
add type information to _repr_ of Macaulay2 elements
Reported by: | Markus Wageringel | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.0 |
Component: | interfaces: optional | Keywords: | macaulay2 |
Cc: | Dima Pasechnik, gh-antonleykin | Merged in: | |
Authors: | Markus Wageringel | Reviewers: | Dima Pasechnik, Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | 2324c95 (Commits, GitHub, GitLab) | Commit: | 2324c95431269e4d7ab5f0c4653a70442c36d652 |
Dependencies: | Stopgaps: |
Description
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
Change History (18)
comment:1 Changed 3 years ago by
comment:2 Changed 3 years ago by
Type: | PLEASE CHANGE → enhancement |
---|
comment:3 Changed 3 years ago by
Authors: | → Markus Wageringel |
---|---|
Branch: | → u/gh-mwageringel/28304 |
Cc: | Dima Pasechnik gh-antonleykin added |
Commit: | → fd3ab693e19ef0c70d67a4ac73870d4809c37664 |
Status: | new → needs_review |
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:
e25e16b | 28304: add after_print_text() for Macaulay2 elements
|
fd3ab69 | 28304: add global options to Macaulay2 to enable AfterPrint
|
comment:4 Changed 3 years ago by
Milestone: | sage-8.9 → sage-9.0 |
---|
comment:5 Changed 3 years ago by
Commit: | fd3ab693e19ef0c70d67a4ac73870d4809c37664 → bcbf9aafa6e6977be1866f6973ef8ecba8bb37e7 |
---|
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
bcbf9aa | 28304: add global options to Macaulay2 to enable AfterPrint
|
comment:6 Changed 3 years ago by
Reviewers: | → Dima Pasechnik |
---|---|
Status: | needs_review → needs_work |
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.
comment:7 Changed 3 years ago by
Commit: | bcbf9aafa6e6977be1866f6973ef8ecba8bb37e7 → 3fc7b990ececd87325e0d56ada938ec2f886fe97 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
3fc7b99 | 28304: improve Macaulay2 documentation
|
comment:8 Changed 3 years ago by
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.
comment:9 Changed 3 years ago by
Status: | needs_work → needs_review |
---|
comment:10 follow-up: 11 Changed 3 years ago by
For the after_print
option, IMO more natural values would be True
and False
rather than strings.
comment:11 follow-up: 12 Changed 3 years ago by
Replying to tscrim:
For the
after_print
option, IMO more natural values would beTrue
andFalse
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.
comment:12 follow-up: 14 Changed 3 years ago by
Replying to gh-mwageringel:
Replying to tscrim:
For the
after_print
option, IMO more natural values would beTrue
andFalse
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))
comment:13 Changed 3 years ago by
Commit: | 3fc7b990ececd87325e0d56ada938ec2f886fe97 → 2324c95431269e4d7ab5f0c4653a70442c36d652 |
---|
Branch pushed to git repo; I updated commit sha1. New commits:
2324c95 | 28304: use booleans for after_print option
|
comment:14 Changed 3 years ago by
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.
comment:15 Changed 3 years ago by
Dima, are you going to finish the review here or do you want me to do it?
comment:16 Changed 3 years ago by
Hi Travis, please go ahead with your review. I am a bit pressed for time now.
comment:17 Changed 3 years ago by
Reviewers: | Dima Pasechnik → Dima Pasechnik, Travis Scrimshaw |
---|---|
Status: | needs_review → positive_review |
Okay, LGTM.
comment:18 Changed 3 years ago by
Branch: | u/gh-mwageringel/28304 → 2324c95431269e4d7ab5f0c4653a70442c36d652 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
With #28303, it would be easy to implement this as follows:
src/sage/interfaces/macaulay2.py
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).