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.

Sign up for GitHub

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

Jump to bottom

Let Sage honour PEP 515 (py3) #28490

Closed
sagetrac-tmonteil mannequin opened this issue Sep 14, 2019 · 61 comments
Closed

Let Sage honour PEP 515 (py3) #28490

sagetrac-tmonteil mannequin opened this issue Sep 14, 2019 · 61 comments
Labels
c: python3 p: major / 3 t: enhancement
Milestone
sage-9.1

Comments

@sagetrac-tmonteil
Copy link
Mannequin

sagetrac-tmonteil mannequin commented Sep 14, 2019

In Python 3, underscored numbers like 10_000_000.0 are allowed, but the preparser, as well as integer and real number constructors, do not handle them yet, see this Ask Sage question and PEP 515.

CC: @slel

Component: python3

Keywords: preparser

Author: Vincent Delecroix, John Palmieri

Branch/Commit: 4c0787d

Reviewer: Thierry Monteil, Vincent Delecroix, John Palmieri

Issue created by migration from https://trac.sagemath.org/ticket/28490
@sagetrac-tmonteil sagetrac-tmonteil mannequin added this to the sage-8.9 milestone Sep 14, 2019
@slel
Copy link
Member

slel commented Sep 14, 2019

Changed keywords from none to preparser

@slel

This comment has been minimized.

Sign in to view
@jhpalmieri
Copy link
Member

Branch: u/jhpalmieri/preparse_underscore

@jhpalmieri
Copy link
Member

Commit: 3601e61

@jhpalmieri
Copy link
Member

Author: John Palmieri

@jhpalmieri
Copy link
Member

comment:3

Here is an attempt. This does at least one other thing: it no longer allows leading zeroes in integers. This is not valid Python syntax, so Sage should not allow it, either.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 18, 2019

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

cfcdc57trac 28490: preparse underscores in numeric literals.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 18, 2019

Changed commit from 3601e61 to cfcdc57

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Oct 18, 2019

comment:5

In my understanding of the PEP (confirmed by testing on a ipython3 console), trailing underscores are not allowed, however we currently have:

sage: 100_000_
100000

Also, perhaps could you show the feature in action with a concrete example, something like:

The preparser can handle PEP 515 (see :trac:`28490`)::

    sage: 1_000_000 + 3_000
    1003000

@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Oct 18, 2019

Reviewer: Thierry Monteil

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 19, 2019

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

61f0618trac 28490: do not allow trailing underscores

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 19, 2019

Changed commit from cfcdc57 to 61f0618

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 19, 2019

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

a26c289trac 28490: do not allow trailing underscores

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 19, 2019

Changed commit from 61f0618 to a26c289

@jhpalmieri
Copy link
Member

comment:8

Okay, trailing underscores now should fail, and I added the doctest.

@jhpalmieri

This comment has been minimized.

Sign in to view
@sagetrac-tmonteil
Copy link
Mannequin Author

sagetrac-tmonteil mannequin commented Oct 19, 2019

comment:9

The patchbots seem unhappy, apparently the doctests do not behave the same there.

@jhpalmieri
Copy link
Member

comment:10

The doctests pass with Python 3, not with Python 2. I guess there is a question: using underscores is only valid starting in Python. Do we want to allow this in Sage built with Python 2?

Also, a leading zero is valid in Python 2 but not in Python 3. What should we do about that?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 19, 2019

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

2fe807btrac 28490: fix py2 behavior and doctests

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 19, 2019

Changed commit from a26c289 to 2fe807b

@jhpalmieri
Copy link
Member

comment:12

I propose to allow underscores with Python 2.

@videlec
Copy link
Contributor

videlec commented Feb 25, 2020

comment:31

Replying to @jhpalmieri:

First, with this change:

<SNIP>

preparse(100_000.0) works, but 100_000.0 does not:

<SNIP>

So that's some progress.

Nice! To my mind, this is the end. RealNumber should accept this kind of strings as float does.

sage: float('1_1.1_1e1_1')
1111000000000.0

It is not the job of the preparser to modify the input string.

Then this change fixes the conversion to RealNumber:

<SNIP>

Which is precisely what I don't wnat to do here!

@jhpalmieri
Copy link
Member

comment:32

Replying to @videlec:

Then this change fixes the conversion to RealNumber:

<SNIP>

Which is precisely what I don't wnat to do here!

Why not? This is in the __init__ method for RealNumber, where the string is already modified, as you can see from the surrounding lines. It's not in the preparser. Where else would you want to fix it?

@videlec
Copy link
Contributor

videlec commented Feb 25, 2020

comment:33

Replying to @jhpalmieri:

Replying to @videlec:

Then this change fixes the conversion to RealNumber:

<SNIP>

Which is precisely what I don't wnat to do here!

Why not? This is in the __init__ method for RealNumber, where the string is already modified, as you can see from the surrounding lines. It's not in the preparser. Where else would you want to fix it?

Oups. I misread. It is fine modifying RealNumber. But the title and description has to be modified as it is not only about touching the preparser. Also Integer and Rational constructors have to be fixed.

@videlec
Copy link
Contributor

videlec commented Feb 25, 2020

comment:34

Your 'fix' from the first part of comment:29 was not one... I will update the branch in a second.

@videlec
Copy link
Contributor

videlec commented Feb 25, 2020

Changed branch from u/jhpalmieri/preparse_underscore to public/28490

@videlec
Copy link
Contributor

videlec commented Feb 25, 2020

Changed commit from 8d22780 to 604d18d

@videlec
Copy link
Contributor

videlec commented Feb 25, 2020

New commits:

604d18d28490: let preparser handle correctly _ in numbers

@videlec
Copy link
Contributor

videlec commented Feb 25, 2020

Changed author from John Palmieri to John Palmieri, Vincent Delecroix

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 25, 2020

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

f235d0228490: remove leading 0 doctest
0148e7a28490: more real numbers parser doctests
ca9809328490: remove dec_num
d40ba4428490: fix X.0 -> X.gen(0)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 25, 2020

Changed commit from 604d18d to d40ba44

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 25, 2020

Changed commit from d40ba44 to 70496a9

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 25, 2020

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

70496a928490: integer and real number constructor

@jhpalmieri
Copy link
Member

comment:39

With your branch in Python 2, Integer('1_3') returns 13 but plain 1_3 raises a SyntaxError. Is that what you intended? It's not consistent behavior.

@jhpalmieri jhpalmieri changed the title Let Sage preparser honour PEP 515 (py3) Let Sage honour PEP 515 (py3) Feb 25, 2020
@jhpalmieri

This comment has been minimized.

Sign in to view
@videlec
Copy link
Contributor

videlec commented Feb 26, 2020

comment:42

Replying to @jhpalmieri:

With your branch in Python 2, Integer('1_3') returns 13 but plain 1_3 raises a SyntaxError. Is that what you intended? It's not consistent behavior.

It is not nice I agree. But I do not quite see inconsistency. Some constructors (like Integer.__init__) can perfectly support additional syntax. The problem to me is that on Python2, 1_3. does not raise an error. But I do not want to worry about Python2. If you find a non invasive solution, feel free to add to the branch.

Related comment: it would be interesting to benchmark the two options for the preparser 123 -> Integer(123) to 123 -> Integer('123'). See the noticeable slowdown mentioned in this sage-devel thread.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 26, 2020

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

4c0787dtrac 28490: tag one doctest "py3"

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 26, 2020

Changed commit from 70496a9 to 4c0787d

@jhpalmieri
Copy link
Member

comment:44

Okay, good enough, I think.

@jhpalmieri
Copy link
Member

Changed reviewer from Thierry Monteil to Thierry Monteil, Vincent Delecroix, John Palmieri

@jhpalmieri
Copy link
Member

Changed author from John Palmieri, Vincent Delecroix to Vincent Delecroix, John Palmieri

@vbraun
Copy link
Member

vbraun commented Feb 27, 2020

Changed branch from public/28490 to 4c0787d

@vbraun vbraun closed this as completed in ec0ac6a Feb 27, 2020
@io4 io4 mentioned this issue Dec 14, 2022
Closed
kryzar pushed a commit to kryzar/sage that referenced this issue Feb 6, 2023
Trac sagemath#34819: Numerical separators behave inconsistently
84246cc 
PEP 515 numerical separators (such as 1_000_000, as introduced in
sagemath#28490) behave inconsistently, ignoring everything after the first
separator upon conversion to a different precision.

Reproduction:
{{{
    sage: z = 1_0000.000000000000000000000000000000000000
    sage: z # works as expected without the conversion
    10000.000000000000000000000000000000000000
    sage: RR(z)
    1.00000000000000
}}}

SageMath version 9.5, Release Date: 2022-01-30, Using Python 3.10.6, OS:
Ubuntu 22.04.1

Also reproduces on Sage 9.7

URL: https://trac.sagemath.org/34819
Reported by: gh-io4
Ticket author(s): Lucas Fiegl
Reviewer(s): Marc Mezzarobba
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
c: python3 p: major / 3 t: enhancement
Projects
None yet
Milestone
sage-9.1
Development

No branches or pull requests
5 participants
@embray @vbraun @jhpalmieri @slel @videlec