#11761 closed enhancement (fixed)
Upgrade Cython to 0.15.1
Reported by: | robertwb | Owned by: | tbd |
---|---|---|---|
Priority: | major | Milestone: | sage-4.8 |
Component: | packages: standard | Keywords: | |
Cc: | fbissey, jpflori | Merged in: | sage-4.8.alpha1 |
Authors: | Robert Bradshaw | Reviewers: | Jeroen Demeyer |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
Lots of improvements and bug fixes, including generators.
See http://wiki.cython.org/ReleaseNotes-0.15.1 for details.
New spkg: http://sage.math.washington.edu/home/jdemeyer/spkg/cython-0.15.1.spkg
Apply 11761-cython-0.15-rebased.patch and 11761_review.patch to the Sage library.
Attachments (4)
Change History (36)
comment:1 in reply to: ↑ description Changed 6 years ago by
Changed 6 years ago by
comment:2 Changed 6 years ago by
- Status changed from new to needs_review
comment:3 Changed 6 years ago by
- Cc fbissey added
comment:4 Changed 6 years ago by
- Cc jpflori added
comment:5 Changed 6 years ago by
Note that 0.15.1 is around the corner, but it's not as big of a jump.
comment:6 Changed 6 years ago by
- Description modified (diff)
Would 0.15.1 require additional changes to Sage?
comment:7 follow-up: ↓ 8 Changed 6 years ago by
Just FYI, 0.15.1 is now released.
comment:8 in reply to: ↑ 7 ; follow-up: ↓ 9 Changed 6 years ago by
Replying to jason:
Just FYI, 0.15.1 is now released.
Oh, did you tell Robert?
(Although he said "it's not as big of a jump".)
comment:9 in reply to: ↑ 8 ; follow-up: ↓ 10 Changed 6 years ago by
comment:10 in reply to: ↑ 9 Changed 6 years ago by
comment:11 Changed 6 years ago by
- Summary changed from Upgrade Cython to 0.15 to Upgrade Cython to 0.15 or later
:)
comment:12 Changed 6 years ago by
- Description modified (diff)
- Summary changed from Upgrade Cython to 0.15 or later to Upgrade Cython to 0.15.1
No further patches needed for 0.15.1.
Changed 6 years ago by
comment:13 Changed 6 years ago by
- Description modified (diff)
- Milestone changed from sage-4.7.2 to sage-4.7.3
comment:14 Changed 6 years ago by
Minor comment: spkg-install
is not executable (I know it is made executable upon building, but better fix this in the spkg anyway).
comment:15 Changed 6 years ago by
- Description modified (diff)
comment:16 Changed 6 years ago by
- Status changed from needs_review to needs_work
Why is the directory tests
missing from the Cython source? This should either be fixed, or be documented.
comment:17 Changed 6 years ago by
What should be done with:
cdef mpz_t denom mpz_init_set_si(denom, 1)
This code is correct but gives a local variable 'denom' referenced before assignment
warning.
(there are many of these in sage/matrix/misc.pyx
for example)
comment:18 Changed 6 years ago by
- Description modified (diff)
- Reviewers set to Jeroen Demeyer
comment:19 Changed 6 years ago by
This new Cython really gives a lot more warnings than the old one, in many cases for code which is perfectly fine. For me, this isn't really a problem, but it is certainly annoying.
comment:20 follow-ups: ↓ 21 ↓ 22 ↓ 24 Changed 6 years ago by
- Status changed from needs_work to needs_review
I've fixed the executable bit on spkg-install and added the tests to the spkg. (I remember someone complaining way back that they were too big, but I don't think it's an issue and we've regularly shipped them before, and it's easier to just have them there.) The spkg at the above link has been refreshed.
The warnings are primarily due to the fact that mpz_t
et al are declared as void*
rather than arrays of size 1. http://hg.sagemath.org/sage-main/file/2a2abbcad325/sage/libs/gmp/types.pxd#l1 Once this is fixed the warnings should go away (and they are correct with respect to the current declarations). All the other warnings I've noticed are correct too, I simply haven't had the time to fix/update Sage's code.
Your review patch looks fine. You obviously looked at this, is the window closed for 4.7.2?
comment:21 in reply to: ↑ 20 Changed 6 years ago by
Replying to robertwb:
Your review patch looks fine.
In fact, it doesn't. I removed one line too much, fixed now.
You obviously looked at this, is the window closed for 4.7.2?
This will certainly not be merged in sage-4.7.2.
Changed 6 years ago by
comment:22 in reply to: ↑ 20 Changed 6 years ago by
- Status changed from needs_review to positive_review
Replying to robertwb:
The warnings are primarily due to the fact that
mpz_t
et al are declared asvoid*
rather than arrays of size 1. http://hg.sagemath.org/sage-main/file/2a2abbcad325/sage/libs/gmp/types.pxd#l1 Once this is fixed the warnings should go away (and they are correct with respect to the current declarations).
If there is an easy way to fix this, then we should do this. I'm saying we should fix every single warning but if it is possible to declare mpz_t
such that there are no warnings, we should certainly do this.
comment:23 Changed 6 years ago by
Of course, that should have been "I'm not saying we should fix every single warning"
comment:24 in reply to: ↑ 20 Changed 6 years ago by
- Status changed from positive_review to needs_work
Replying to robertwb:
I've fixed the executable bit on spkg-install and added the tests to the spkg.
One more thing: changing permissions is a change which should be committed. So could you please do a hg commit
and re-upload the spkg? Otherwise, the package and patch looks good.
comment:26 follow-up: ↓ 28 Changed 6 years ago by
- Milestone set to sage-4.8
- Status changed from needs_work to needs_review
Spkg updated with executable bit set commit.
comment:27 Changed 6 years ago by
Yes, I agree these warnings should be fixed. #11992 .
comment:28 in reply to: ↑ 26 Changed 6 years ago by
Replying to robertwb:
Spkg updated with executable bit set commit.
Looks you forgot to update the spkg, it is exactly the same as before (last modified on 1 november):
$ ls -l /home/robertwb/cython/spkg/cython-0.15.1.spkg -rw-r--r-- 1 robertwb robertwb 1420704 2011-11-01 06:57 /home/robertwb/cython/spkg/cython-0.15.1.spkg
Never mind, I will make the change.
comment:29 Changed 6 years ago by
- Description modified (diff)
- Status changed from needs_review to positive_review
comment:30 Changed 6 years ago by
Oops, I put it at
robertwb@sage:~/cython$ ls -l /home/robertwb/cython/cython-0.15.1.spkg -rw-r--r-- 1 robertwb robertwb 1412114 2011-11-04 22:51 /home/robertwb/cython/cython-0.15.1.spkg
That's what comes of getting too little sleep. Go ahead and use that one (or if you've already done the trivial change, even better.)
comment:31 Changed 6 years ago by
- Merged in set to sage-4.8.alpha1
- Resolution set to fixed
- Status changed from positive_review to closed
comment:32 Changed 6 years ago by
Sorry to be late to the party. I get an error building the optional mpc extension in 4.8.alpha2 and I suspect cython-0.15.1 is the problem
sage/rings/complex_mpc.c: In function '__pyx_pf_4sage_5rings_11complex_mpc_15MPComplexNumber_16__float__': sage/rings/complex_mpc.c:9720:48: error: cast specifies array type
I am guessing the complex_mpc.pyx code needs to be updated but I don't really know how.
Replying to robertwb:
Yeah!