Opened 5 years ago

Closed 4 years ago

#24414 closed defect (fixed)

py3: sage.libs.singular fixes

Reported by: embray Owned by:
Priority: major Milestone: sage-8.2
Component: python3 Keywords: singular
Cc: Merged in:
Authors: Erik Bray Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 7b3a118 (Commits, GitHub, GitLab) Commit: 7b3a118f6f3ade74c0ce6ba85d28a4d80233230f
Dependencies: Stopgaps:

Status badges

Description

With these fixes, plus a few unrelated fixes in other modules (to be submitted separately) I was able to get all of the tests in sage.libs.singular to pass on Python 3.

Change History (21)

comment:1 Changed 5 years ago by embray

  • Keywords singular added
  • Status changed from new to needs_review

comment:2 Changed 4 years ago by chapoton

  • Status changed from needs_review to needs_work

does not apply

comment:3 Changed 4 years ago by git

  • Commit changed from b30ca6fd0bd9be7ea4a145791e5e85e0b289006d to 5bea75d0645a0461420eeb495e7ae9b523cc5699

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​f8983c5Numerous string conversions from bytes to str and from str to bytes for Python 3
​5bea75dVarious Python 3 fixes for sage.libs.singular

comment:4 Changed 4 years ago by git

  • Commit changed from 5bea75d0645a0461420eeb495e7ae9b523cc5699 to 4e514d4c270a02f147f4a961b38d29ebc6f6ec8f

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​4e514d4Various Python 3 fixes for sage.libs.singular

comment:5 Changed 4 years ago by embray

  • Status changed from needs_work to needs_review

comment:6 follow-up: Changed 4 years ago by jdemeyer

In src/sage/libs/singular/ring.pyx, I'm not convinced by the changes to __cmp__. Can we please factor that out to a separate ticket?

comment:7 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:8 Changed 4 years ago by jdemeyer

  • Dependencies #24223 deleted

comment:9 in reply to: ↑ 6 ; follow-up: Changed 4 years ago by embray

Replying to jdemeyer:

In src/sage/libs/singular/ring.pyx, I'm not convinced by the changes to __cmp__.

I'm not either, now that you call it out. I think this might have just been a placeholder for the correct implementation, but it definitely doesn't look correct as is.

Can we please factor that out to a separate ticket?

I'd rather not. It's still only going to be a small change. It does need more tests though.

comment:10 Changed 4 years ago by embray

I think instead of __richcmp__ (or __cmp__ for that matter) all this needs is an __eq__.

comment:11 in reply to: ↑ 9 Changed 4 years ago by jdemeyer

Replying to embray:

It's still only going to be a small change.

"small" in which metric? Number of lines of code? Changing __cmp__ to __richcmp__ is a non-trivial change.

comment:12 Changed 4 years ago by embray

Not in this case it isn't.

comment:13 Changed 4 years ago by git

  • Commit changed from 4e514d4c270a02f147f4a961b38d29ebc6f6ec8f to 94c07ae51b553366427d511daca613f01372e60a

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

​94c07aeThis __richcmp__ really only needs to support == and != for use of these objects as dict keys.

comment:14 Changed 4 years ago by embray

  • Status changed from needs_work to needs_review

For some more complicated class I might agree that it should have a ticket focused on getting its __cmp__ -> __richcmp__ conversion right. But in this case it's just a little internal class used to wrap a pointer to a Singular ring structure so that it can be used as a dict key.

It really only needs __eq__ defined for this purpose and it's pretty trivial IMO.

comment:15 Changed 4 years ago by chapoton

green bot

comment:16 Changed 4 years ago by jdemeyer

OK, you convinced me about __richcmp__. I'm preparing a small reviewer commit...

comment:17 Changed 4 years ago by jdemeyer

  • Branch changed from u/embray/python3/sage-libs-singular to u/jdemeyer/python3/sage-libs-singular

comment:18 Changed 4 years ago by git

  • Commit changed from 94c07ae51b553366427d511daca613f01372e60a to 7b3a118f6f3ade74c0ce6ba85d28a4d80233230f

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​7b3a118Reviewer fixes to libsingular interface

comment:19 Changed 4 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer

Please review.

comment:20 Changed 4 years ago by embray

  • Status changed from needs_review to positive_review

Ah, 

return (self._ring == r._ring) == (op == Py_EQ)

I originally had almost this but with an and instead of an == but when I realized that was wrong I somehow overlooked this solution.

Some of the other changes are weird because I already had them in my local Python 3 branch. Maybe I made them after I first opened this ticket.

comment:21 Changed 4 years ago by vbraun

  • Branch changed from u/jdemeyer/python3/sage-libs-singular to 7b3a118f6f3ade74c0ce6ba85d28a4d80233230f
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.