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: |
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
- Keywords singular added
- Status changed from new to needs_review
comment:2 Changed 4 years ago by
- Status changed from needs_review to needs_work
comment:3 Changed 4 years ago by
- Commit changed from b30ca6fd0bd9be7ea4a145791e5e85e0b289006d to 5bea75d0645a0461420eeb495e7ae9b523cc5699
comment:4 Changed 4 years ago by
- Commit changed from 5bea75d0645a0461420eeb495e7ae9b523cc5699 to 4e514d4c270a02f147f4a961b38d29ebc6f6ec8f
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
4e514d4 | Various Python 3 fixes for sage.libs.singular
|
comment:5 Changed 4 years ago by
- Status changed from needs_work to needs_review
comment:6 follow-up: ↓ 9 Changed 4 years ago by
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
- Status changed from needs_review to needs_work
comment:8 Changed 4 years ago by
- Dependencies #24223 deleted
comment:9 in reply to: ↑ 6 ; follow-up: ↓ 11 Changed 4 years ago by
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
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
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
Not in this case it isn't.
comment:13 Changed 4 years ago by
- Commit changed from 4e514d4c270a02f147f4a961b38d29ebc6f6ec8f to 94c07ae51b553366427d511daca613f01372e60a
Branch pushed to git repo; I updated commit sha1. New commits:
94c07ae | This __richcmp__ really only needs to support == and != for use of these objects as dict keys.
|
comment:14 Changed 4 years ago by
- 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
green bot
comment:16 Changed 4 years ago by
OK, you convinced me about __richcmp__
. I'm preparing a small reviewer commit...
comment:17 Changed 4 years ago by
- Branch changed from u/embray/python3/sage-libs-singular to u/jdemeyer/python3/sage-libs-singular
comment:18 Changed 4 years ago by
- Commit changed from 94c07ae51b553366427d511daca613f01372e60a to 7b3a118f6f3ade74c0ce6ba85d28a4d80233230f
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
7b3a118 | Reviewer fixes to libsingular interface
|
comment:20 Changed 4 years ago by
- 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
- Branch changed from u/jdemeyer/python3/sage-libs-singular to 7b3a118f6f3ade74c0ce6ba85d28a4d80233230f
- Resolution set to fixed
- Status changed from positive_review to closed
does not apply