Opened 4 years ago
Closed 3 years ago
#26403 closed enhancement (fixed)
Compile Sage with Cython language_level=3str
Reported by: | jdemeyer | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.0 |
Component: | cython | Keywords: | |
Cc: | embray, chapoton | Merged in: | |
Authors: | Jeroen Demeyer | Reviewers: | Frédéric Chapoton |
Report Upstream: | N/A | Work issues: | |
Branch: | 58a30a8 (Commits, GitHub, GitLab) | Commit: | 58a30a8da95afe7299348443e2669238b5ecfd0d |
Dependencies: | Stopgaps: |
Description
Cython 0.29 introduces a new language_level=3str
, which is like language_level=3
except that strings are still str
instead of unicode
. It would be good to use this language_level in Sage to force Python 3 syntax in all Cython files.
Change History (36)
comment:1 follow-up: ↓ 3 Changed 4 years ago by
comment:2 follow-up: ↓ 4 Changed 4 years ago by
Regarding your deleted comment from the other ticket (maybe it was meant for this one) about non-ASCII strings in string literals (there was an example you used with μ). If a docstring contains non-ASCII characters then I definitely think we should be explicitly using u''
.
comment:3 in reply to: ↑ 1 ; follow-up: ↓ 6 Changed 4 years ago by
Replying to embray:
I am still confused by the whole language_level discussion. I tried following the discussion on the cython mailing list but I gave up.
If I understand correctly (and this is probably just restating what you wrote in the ticket description)
language_level=3str
means it's using Python 3 syntax in all Cython files, with the exception that unprefixed string literals are non-unicode strings on Python 2?
It's not only about string literals. With language_level=3
, the global str
means unicode
:
sage: cython(''' ....: # cython: language_level=3 ....: print(str) ....: ''') <type 'unicode'>
comment:4 in reply to: ↑ 2 ; follow-up: ↓ 5 Changed 4 years ago by
Replying to embray:
If a docstring contains non-ASCII characters then I definitely think we should be explicitly using
u''
.
On the other hand, I do consider it a bug that Cython disallows "μ"
with language_level=3str
since that's valid in both Python 2 and Python 3 (with a different meaning though).
comment:5 in reply to: ↑ 4 Changed 4 years ago by
Replying to jdemeyer:
Replying to embray:
If a docstring contains non-ASCII characters then I definitely think we should be explicitly using
u''
.On the other hand, I do consider it a bug that Cython disallows
"μ"
withlanguage_level=3str
since that's valid in both Python 2 and Python 3 (with a different meaning though).
Sure it's technically valid on Python 2, but not a great practice in most cases either, and with the different meaning there's a lot of potential for confusion around it so I think they're right to disallow it (even if it's unintentionally disallowed). But I also see your point.
comment:6 in reply to: ↑ 3 ; follow-up: ↓ 7 Changed 4 years ago by
Replying to jdemeyer:
Replying to embray:
I am still confused by the whole language_level discussion. I tried following the discussion on the cython mailing list but I gave up.
If I understand correctly (and this is probably just restating what you wrote in the ticket description)
language_level=3str
means it's using Python 3 syntax in all Cython files, with the exception that unprefixed string literals are non-unicode strings on Python 2?It's not only about string literals. With
language_level=3
, the globalstr
meansunicode
:sage: cython(''' ....: # cython: language_level=3 ....: print(str) ....: ''') <type 'unicode'>
Ok, sure. But "3str" means str
is just always str
right? I guess I like that approach if so; it meshes well with the approach we've been already taking in Sage.
comment:7 in reply to: ↑ 6 Changed 4 years ago by
Replying to embray:
Ok, sure. But "3str" means
str
is just alwaysstr
right?
Yes, str
is str
and "literals"
are also of type str
.
comment:8 Changed 4 years ago by
- Dependencies changed from #25292 to #25292, #26413
comment:9 Changed 4 years ago by
- Dependencies changed from #25292, #26413 to #25292, #26413, #26414
comment:10 Changed 4 years ago by
- Branch set to u/jdemeyer/compile_sage_with_cython_language_level_3str
comment:11 Changed 4 years ago by
- Commit set to 0ea70b5ea17f62a37e601cf6ed9802c9b3d16b22
comment:12 Changed 4 years ago by
- Commit changed from 0ea70b5ea17f62a37e601cf6ed9802c9b3d16b22 to b3a8a8838f6a5fdf5b8bcd6c8c9000a710f10c6c
comment:13 Changed 3 years ago by
- Commit changed from b3a8a8838f6a5fdf5b8bcd6c8c9000a710f10c6c to 172bb57b991f7b90a006a20fb781da38af2e8bb4
comment:14 Changed 3 years ago by
- Dependencies #25292, #26413, #26414 deleted
- Milestone changed from sage-8.4 to sage-8.7
comment:15 Changed 3 years ago by
- Commit changed from 172bb57b991f7b90a006a20fb781da38af2e8bb4 to b821f77d81822d2efee48385df5c58a4c619e07c
comment:16 Changed 3 years ago by
- Milestone changed from sage-8.7 to sage-8.8
Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)
comment:17 Changed 3 years ago by
red branch
comment:18 Changed 3 years ago by
I know, but it's not ready for review anyway. This needs to wait for Cython 3.0
comment:19 Changed 3 years ago by
- Milestone sage-8.8 deleted
As the Sage-8.8 release milestone is pending, we should delete the sage-8.8 milestone for tickets that are not actively being worked on or that still require significant work to move forward. If you feel that this ticket should be included in the next Sage release at the soonest please set its milestone to the next release milestone (sage-8.9).
comment:20 Changed 3 years ago by
- Commit changed from b821f77d81822d2efee48385df5c58a4c619e07c to ae1f7f451251f40f3eb2e73adcfb5055fd273644
comment:21 Changed 3 years ago by
- Status changed from new to needs_review
comment:22 Changed 3 years ago by
red branch. I will review if you merge
comment:23 Changed 3 years ago by
- Status changed from needs_review to needs_work
comment:24 Changed 3 years ago by
I think this just needs a rebase + testing again. This should be done as a dependency of #28000.
comment:25 Changed 3 years ago by
- Branch changed from u/jdemeyer/compile_sage_with_cython_language_level_3str to public/ticket/26403
- Commit changed from ae1f7f451251f40f3eb2e73adcfb5055fd273644 to 5a330134f1f77a8c6b2d039684584edf8f145d51
- Status changed from needs_work to needs_review
branch merged with 8.9.rc0
New commits:
5a33013 | Merge branch 'u/jdemeyer/compile_sage_with_cython_language_level_3str' in 8.9.rc0
|
comment:26 Changed 3 years ago by
Three patchbots seems to be happy, both with py2 and py3. Erik, would you approve a positive review ?
comment:27 follow-up: ↓ 30 Changed 3 years ago by
Could please somebody else approve ? Or should I set to positive all by myself ?
comment:28 Changed 3 years ago by
- Milestone set to sage-9.0
- Reviewers set to Frédéric Chapoton
- Status changed from needs_review to positive_review
In the absence of any reaction whatsoever, I am setting to positive.
comment:29 Changed 3 years ago by
- Status changed from positive_review to needs_work
merge conflict...
comment:30 in reply to: ↑ 27 Changed 3 years ago by
Replying to chapoton:
Could please somebody else approve ? Or should I set to positive all by myself ?
Yeah why shouldn't you? You reviewed the change and tested it, didn't you? You don't need me to do that.
comment:31 Changed 3 years ago by
- Commit changed from 5a330134f1f77a8c6b2d039684584edf8f145d51 to 0a34ee961537652de9e61352670b2b6383be3d70
Branch pushed to git repo; I updated commit sha1. New commits:
0a34ee9 | Merge branch 'public/ticket/26403' in 9.0.b1
|
comment:32 Changed 3 years ago by
- Status changed from needs_work to positive_review
conflicts resolved, setting back to positive
comment:33 Changed 3 years ago by
- Status changed from positive_review to needs_work
I'm getting the following failure on 32-bit:
File "src/sage/plot/plot3d/shapes.pyx", line 947, in sage.plot.plot3d.shapes.Sphere.get_grid Failed example: Sphere(1).get_grid(100) Expected: ([-10.0, ..., 0.0, ..., 10.0], [0.0, ..., 3.141592653589793, ..., 0.0]) Got: ([-10.0, -1.0471975511965976, -0.5235987755982988, 6.1257422745431e-17, 0.5235987755982989, 1.0471975511965979, 10.0], [0.0, 1.0471975511965979, 2.0943951023931957, 3.141592653589793, 4.188790204786391, 5.235987755982989, 0.0]) ********************************************************************** 1 item had failures: 1 of 3 in sage.plot.plot3d.shapes.Sphere.get_grid [162 tests, 1 failure, 43.51 s] ---------------------------------------------------------------------- sage -t --long src/sage/plot/plot3d/shapes.pyx # 1 doctest failed ----------------------------------------------------------------------
comment:34 Changed 3 years ago by
- Commit changed from 0a34ee961537652de9e61352670b2b6383be3d70 to 58a30a8da95afe7299348443e2669238b5ecfd0d
Branch pushed to git repo; I updated commit sha1. New commits:
58a30a8 | trac 26403 fix 32bit doctest
|
comment:36 Changed 3 years ago by
- Branch changed from public/ticket/26403 to 58a30a8da95afe7299348443e2669238b5ecfd0d
- Resolution set to fixed
- Status changed from positive_review to closed
I am still confused by the whole language_level discussion. I tried following the discussion on the cython mailing list but I gave up.
If I understand correctly (and this is probably just restating what you wrote in the ticket description)
language_level=3str
means it's using Python 3 syntax in all Cython files, with the exception that unprefixed string literals are non-unicode strings on Python 2?