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

py3: let the repr of numbers be a string #23812

Closed
fchapoton opened this issue Sep 9, 2017 · 30 comments
Closed

py3: let the repr of numbers be a string #23812

fchapoton opened this issue Sep 9, 2017 · 30 comments

Comments

@fchapoton
Copy link
Contributor

and not forced to be "bytes"

replaced by #24223

CC: @embray @jdemeyer

Component: python3

Keywords: unicode

Author: Frédéric Chapoton

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

@fchapoton fchapoton added this to the sage-8.1 milestone Sep 9, 2017
@fchapoton
Copy link
Contributor Author

Commit: f34394d

@fchapoton
Copy link
Contributor Author

Branch: u/chapoton/23812

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 9, 2017

Changed commit from f34394d to f093217

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 9, 2017

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

f093217py3: str repr for integers

@jdemeyer
Copy link

comment:3

I wonder if we should have a general function to convert from bytes to str and back in Sage. For example, like in cypari2: https://github.com/defeo/cypari2/blob/master/cypari2/string_utils.pyx

@fchapoton
Copy link
Contributor Author

comment:4

It seems that we also need to change the repr of rational numbers..

@fchapoton
Copy link
Contributor Author

comment:5

ping ?

@jdemeyer
Copy link

comment:6

As I said, I think we should have a general solution instead of this.

@fchapoton
Copy link
Contributor Author

comment:7

There is

src/sage/misc/six.py:def u(x):

but this is not doing exactly that.

@embray
Copy link
Contributor

embray commented Sep 12, 2017

comment:8

I don't think this is the right answer. There a dozens of places where a similar conversion is needed. There needs to be a generic function for this kind of thing.

@fchapoton
Copy link
Contributor Author

comment:9

This is maybe not the definitive word. If you feel you can do better quickly, please do.

Nethertheless, there are not so many casting to bytes:

git grep -c "<bytes>" *.pyx
src/sage/libs/ecl.pyx:2
src/sage/libs/homfly.pyx:1
src/sage/libs/pynac/pynac.pyx:1
src/sage/numerical/backends/cvxopt_backend.pyx:1
src/sage/numerical/backends/cvxopt_sdp_backend.pyx:1
src/sage/numerical/backends/ppl_backend.pyx:1
src/sage/rings/integer.pyx:1

and integer.pyx is certainly the most important place where we need to correct the casting to bytes.

@jdemeyer
Copy link

comment:10

Replying to @fchapoton:

Nethertheless, there are not so many casting to bytes:

I think most of the time, the bytes is implicit because it's just what corresponds to char * in C. For example, in code like

def f():
    cdef char* cstr = ....
    return cstr

Cython will automatically convert cstr to Python bytes. Since Sage deals with a lot of C libraries, I would guess that this happens in a lot of places.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 13, 2017

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

60782ffMerge branch 'u/chapoton/23812' in 8.1.b5
6f8a6cftrac 23812 also for rationals
f0bafbbMerge branch 'u/chapoton/23812' of trac.sagemath.org:sage into 23812
8cb8955Merge branch '23812' into integer branch

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 13, 2017

Changed commit from f093217 to 8cb8955

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 13, 2017

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

ad28790more utf8 compatibility

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 13, 2017

Changed commit from 8cb8955 to ad28790

@fchapoton
Copy link
Contributor Author

comment:13

what do you think of the new branch ?

@fchapoton fchapoton changed the title py3: let the repr of integers be a string py3: let the repr of numbers be a string Sep 13, 2017
@fchapoton
Copy link
Contributor Author

comment:14

see also #23648

@embray
Copy link
Contributor

embray commented Sep 15, 2017

comment:15

Yeah, this really needs some opposite of string_to_bytes() from #23648. This isn't a great approach. And although in the vast majority of cases we can assume that the return values in these cases will be in ASCII, it's a bad pattern to repeat, because if anything happens to not return ASCII using this pattern you'll get something like this on Python 2:

>>> str(b'\xe2\x98\x83'.decode('utf8'))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
UnicodeEncodeError: 'ascii' codec can't encode character u'\u2603' in position 0: ordinal not in range(128)

@fchapoton
Copy link
Contributor Author

Changed keywords from none to unicode

@fchapoton
Copy link
Contributor Author

comment:17

I would appreciate if Erik or another expert of unicode issues works on the matter here and all related tickets dealing with unicode adaptations for python3. I understand when one tells me that my solutions are not good, but please then propose another branch.

@embray
Copy link
Contributor

embray commented Oct 23, 2017

comment:18

Replying to @fchapoton:

I would appreciate if Erik or another expert of unicode issues works on the matter here and all related tickets dealing with unicode adaptations for python3. I understand when one tells me that my solutions are not good, but please then propose another branch.

I hear you-- I need to get my Python 3 build working again but I do want to help with this stuff. Please feel free to assign such tickets to me.

@embray embray self-assigned this Oct 23, 2017
@fchapoton
Copy link
Contributor Author

comment:19

I have created a new ticket that just wants to introduce the conversion tools: #24186.

@embray
Copy link
Contributor

embray commented Nov 15, 2017

comment:20

Oops, this patch is now for some reason deleting all of rationals.pyx?

I'm going to see if I can rework this with my new string conversion functions.

@fchapoton
Copy link
Contributor Author

comment:21

Replying to @embray:

Oops, this patch is now for some reason deleting all of rationals.pyx?

probably a known bug in trac git plugin

I'm going to see if I can rework this with my new string conversion functions.

ok. Beware also of #24136

@embray embray modified the milestones: sage-8.1, sage-8.2 Dec 12, 2017
@fchapoton

This comment has been minimized.

@fchapoton
Copy link
Contributor Author

comment:23

let us get rid of this one as duplicate

@fchapoton fchapoton removed this from the sage-8.2 milestone Jan 5, 2018
@fchapoton
Copy link
Contributor Author

comment:24

ping ? any objection to close as invalid ?

@fchapoton
Copy link
Contributor Author

Changed branch from u/chapoton/23812 to none

@fchapoton
Copy link
Contributor Author

Changed commit from ad28790 to none

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants