Skip to content
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

Fix the latex representation of a 1-tuple #19348

Merged
merged 1 commit into from May 19, 2020

Conversation

eric-wieser
Copy link
Member

Without this change, a 1-item tuple looks like a set of parentheses, which is misleading.
This adds the trailing comma (or as appropriate, semicolon) that is typical of 1-tuples in python

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

  • printing
    • tuples of one element now include the usual trailing comma

@sympy-bot
Copy link

sympy-bot commented May 17, 2020

Hi, I am the SymPy bot (v158). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

  • printing

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.7.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

Without this change, a 1-item tuple looks like a set of parentheses, which is misleading.
This adds the trailing comma (or as appropriate, semicolon) that is typical of 1-tuples in python

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->


#### Brief description of what is fixed or changed


#### Other comments


#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* printing
  * tuples of one element now include the usual trailing comma
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@mcpl-sympy
Copy link
Contributor

mcpl-sympy commented May 18, 2020

AFAIK, latex printer aims to represent the common mathematical notation, not Python's. Is it common in field of mathematics to denote monuple (1-tuple) with trailing comma?
In this wikipedia page, monuple is written without trailing comma. This might be a counterexample.

@sylee957
Copy link
Member

I have the same opinion that the original one is fine for LaTeX. However, when I see pretty printings, I see it adds comma separator even if the pretty printing doesn't need to be syntactically correct for python.

@eric-wieser
Copy link
Member Author

Given sympy doesn't really know anything about tuples in the mathematical sense, doesn't it make more sense to render them in the python sense?

Adding this comma makes the latex rendering consistent with the repr, str, and pretty

@mcpl-sympy
Copy link
Contributor

mcpl-sympy commented May 18, 2020

It is true that srepr printer should follow Python syntax, since eval on its result should return the same object. I also agree that sstr should follow the same, since Python's default str function on monuple includes trailing comma. However, I don't think latex printer should follow it.

For example, do we need to print powers like image instead of image, because Python uses '**' as power? Or, do we have to print equalities as image because Python uses '==' for equality? I hardly agree.

I believe that rather than latex following the other printers, pretty should follow latex by excluding the trailing comma.

@eric-wieser
Copy link
Member Author

I'd argue the key difference here is that for x**2 etc what's being printed is a mathematical sympy.Pow object. But when you print a tuple, that's not a sympy object at all.

When I do init_printing(use_latex=True), I'm doing it because I want sympy types to show as latex inside python containers. What I'm not asking for is a mathematical interpretation of the containers themselves. If I wanted that, I'd use sympy.containers.Tuple etc instead, or sympify my tuple first.

So maybe I should change _print_tuple, but leave _print_Tuple with its old behavior?

@mcpl-sympy
Copy link
Contributor

Fair point. Now it sounds reasonable for me...

@eric-wieser eric-wieser force-pushed the fix-tuple-latex branch 2 times, most recently from c1ceaa0 to 81574d1 Compare May 18, 2020 22:01
Comment on lines 209 to +214
def _add_parens(self, s):
return r"\left({}\right)".format(s)

# TODO: merge this with the above, which requires a lot of test changes
def _add_parens_lspace(self, s):
return r"\left( {}\right)".format(s)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit ugly, but I really don't want to sift through the 50ish occurences of \left( in the tests for whether they go through the tuple printer, as it would add tonnes of noise to this PR.

Is the presence of this space deliberate?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding of LaTeX is that the space should never be needed, but I could be wrong. You might want to find a reference one way or another (perhaps ask on tex.stackexchange). If the space isn't needed there's no reason the put it there.

IMO we should aim to make the the output of latex() both human and machine readable, but we don't really do that right now. But even then, I'm not really sure if the space here helps with that.

@asmeurer
Copy link
Member

I like this change. To me a trailing comma is perfectly valid from a mathematical notation point of view, and it is a good idea to include it because it disambiguates it from what otherwise appears at first glance to just be redundant parentheses, especially since a Tuple might often appear nested inside of some other expression.

Also, I think most places where Tuple is used and would be printed, it is more of a code like object rather than a mathematical tuple. Where are the places where Tuple is used in another expression, and the Tuple printer is used for it? The only thing I can think of is multi-dimensional sets. Apparently, ProductSet does allow a single argument, so this can happen, and I think the comma is a good way to indicate that the elements are indeed 1-tuples and not just the items in those tuples.

@asmeurer
Copy link
Member

For example:

>>> print(latex(ConditionSet(x, Eq(x, (1,)))))
\left\{x \mid x = \left( 1\right) \right\}

This is perhaps a bit contrived, but I think it shows that 1-tuples can appear in mathematical expressions. Seeing "x = (1)" is confusing if you don't expect a tuple, because it looks like it should just mean "x = 1", whereas "x = (1,)" is clearer.

Without this change, a 1-item tuple looks like a set of parentheses, which is misleading.
This adds the trailing comma (or as appropriate, semicolon) that is typical of 1-tuples in python.
@eric-wieser
Copy link
Member Author

Travis had a network issue, that was to restart it

@jksuom
Copy link
Member

jksuom commented May 19, 2020

It is usually better to wait for someone to restart the failing job.

@eric-wieser
Copy link
Member Author

In this case it was one of the baseline jobs, so it only meant rerunning a handful of tests.

@jksuom
Copy link
Member

jksuom commented May 19, 2020

In that case, it should be ok.

@codecov
Copy link

codecov bot commented May 19, 2020

Codecov Report

Merging #19348 into master will decrease coverage by 0.018%.
The diff coverage is 100.000%.

@@              Coverage Diff              @@
##            master    #19348       +/-   ##
=============================================
- Coverage   75.611%   75.593%   -0.019%     
=============================================
  Files          651       651               
  Lines       169482    169515       +33     
  Branches     39987     39997       +10     
=============================================
- Hits        128148    128142        -6     
- Misses       35722     35756       +34     
- Partials      5612      5617        +5     

@jksuom jksuom merged commit 93a193f into sympy:master May 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants