Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#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:

Attachments (4)

11761-cython-0.15.patch (11.8 KB) - added by robertwb 3 years ago.
11761-cython-0.15-rebased.patch (10.9 KB) - added by jdemeyer 3 years ago.
cython-0.15.1.diff (1.7 KB) - added by jdemeyer 3 years ago.
Diff for the new Cython spkg, for review only
11761_review.patch (3.6 KB) - added by jdemeyer 3 years ago.

Download all attachments as: .zip

Change History (36)

comment:1 in reply to: ↑ description Changed 3 years ago by leif

Replying to robertwb:

... including generators.

Yeah!

Changed 3 years ago by robertwb

comment:2 Changed 3 years ago by robertwb

  • Status changed from new to needs_review

comment:3 Changed 3 years ago by fbissey

  • Cc fbissey added

comment:4 Changed 3 years ago by jpflori

  • Cc jpflori added

comment:5 Changed 3 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 3 years ago by leif

  • Authors set to Robert Bradshaw
  • Description modified (diff)

Would 0.15.1 require additional changes to Sage?

comment:7 follow-up: Changed 3 years ago by jason

Just FYI, 0.15.1 is now released.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 3 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: Changed 3 years ago by jason

Replying to leif:

Replying to jason:

Just FYI, 0.15.1 is now released.

Oh, did you tell Robert?

Oh, I'm sure he knows, since he's the BFDL for Cython, if there is such a position ;).

(Although he said "it's not as big of a jump".)

comment:10 in reply to: ↑ 9 Changed 3 years ago by leif

Replying to jason:

Replying to leif:

Replying to jason:

Just FYI, 0.15.1 is now released.

Oh, did you tell Robert?

Oh, I'm sure he knows, since he's the BFDL for Cython, if there is such a position ;).

I was just kidding.

(Wonder whether he updates the spkg... :P)

comment:11 Changed 3 years ago by leif

  • Summary changed from Upgrade Cython to 0.15 to Upgrade Cython to 0.15 or later

:)

comment:12 Changed 3 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 3 years ago by jdemeyer

comment:13 Changed 3 years ago by jdemeyer

  • Description modified (diff)
  • Milestone changed from sage-4.7.2 to sage-4.7.3

Changed 3 years ago by jdemeyer

Diff for the new Cython spkg, for review only

comment:14 Changed 3 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 3 years ago by jdemeyer

  • Description modified (diff)

comment:16 Changed 3 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 3 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 3 years ago by jdemeyer

  • Description modified (diff)
  • Reviewers set to Jeroen Demeyer

comment:19 Changed 3 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: Changed 3 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 3 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 3 years ago by jdemeyer

comment:22 in reply to: ↑ 20 Changed 3 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 3 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 3 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 3 years ago by jdemeyer

  • Milestone sage-4.7.3 deleted

Milestone sage-4.7.3 deleted

comment:26 follow-up: Changed 3 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 3 years ago by robertwb

Yes, I agree these warnings should be fixed. #11992 .

comment:28 in reply to: ↑ 26 Changed 3 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 3 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from needs_review to positive_review

comment:30 Changed 3 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 3 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 3 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.

Note: See TracTickets for help on using tickets.