#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 jdemeyer)
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 5 years ago by leif
Changed 5 years ago by robertwb
comment:2 Changed 5 years ago by robertwb
- Status changed from new to needs_review
comment:3 Changed 5 years ago by fbissey
- Cc fbissey added
comment:4 Changed 5 years ago by jpflori
- Cc jpflori added
comment:5 Changed 5 years ago by robertwb
Note that 0.15.1 is around the corner, but it's not as big of a jump.
comment:6 Changed 5 years ago by leif
- Description modified (diff)
Would 0.15.1 require additional changes to Sage?
comment:7 follow-up: ↓ 8 Changed 5 years ago by jason
Just FYI, 0.15.1 is now released.
comment:8 in reply to: ↑ 7 ; follow-up: ↓ 9 Changed 5 years ago by leif
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 5 years ago by jason
comment:10 in reply to: ↑ 9 Changed 5 years ago by leif
comment:11 Changed 5 years ago by leif
- Summary changed from Upgrade Cython to 0.15 to Upgrade Cython to 0.15 or later
:)
comment:12 Changed 5 years ago by robertwb
- 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 5 years ago by jdemeyer
comment:13 Changed 5 years ago by jdemeyer
- Description modified (diff)
- Milestone changed from sage-4.7.2 to sage-4.7.3
comment:14 Changed 5 years ago by jdemeyer
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 5 years ago by jdemeyer
- Description modified (diff)
comment:16 Changed 5 years ago by jdemeyer
- 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 5 years ago by jdemeyer
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 5 years ago by jdemeyer
- Description modified (diff)
- Reviewers set to Jeroen Demeyer
comment:19 Changed 5 years ago by jdemeyer
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 5 years ago by robertwb
- 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 5 years ago by jdemeyer
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 5 years ago by jdemeyer
comment:22 in reply to: ↑ 20 Changed 5 years ago by jdemeyer
- 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 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).
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 5 years ago by jdemeyer
Of course, that should have been "I'm not saying we should fix every single warning"
comment:24 in reply to: ↑ 20 Changed 5 years ago by jdemeyer
- 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:25 Changed 5 years ago by jdemeyer
- Milestone sage-4.7.3 deleted
Milestone sage-4.7.3 deleted
comment:26 follow-up: ↓ 28 Changed 5 years ago by robertwb
- Milestone set to sage-4.8
- Status changed from needs_work to needs_review
Spkg updated with executable bit set commit.
comment:27 Changed 5 years ago by robertwb
Yes, I agree these warnings should be fixed. #11992 .
comment:28 in reply to: ↑ 26 Changed 5 years ago by jdemeyer
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 5 years ago by jdemeyer
- Description modified (diff)
- Status changed from needs_review to positive_review
comment:30 Changed 5 years ago by robertwb
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 5 years ago by jdemeyer
- Merged in set to sage-4.8.alpha1
- Resolution set to fixed
- Status changed from positive_review to closed
comment:32 Changed 5 years ago by fbissey
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!