#24740 closed defect (fixed)
py3: numerous string fixes to sage.numerical.backends
Reported by: | embray | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.6 |
Component: | python3 | Keywords: | |
Cc: | Merged in: | ||
Authors: | Erik Bray | Reviewers: | Frédéric Chapoton |
Report Upstream: | N/A | Work issues: | |
Branch: | da920d5 (Commits, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description
Trying to make progress on my backlog of Python 3 fixes I haven't made tickets for...
This one is slightly questionable in that there were many cpdef
functions in this package that take char *
arguments, and I changed them to take non-typed arguments (effectively str
).
I'm not exactly sure what implication this has for the C interfaces that were exported for these methods, or if we care.
Change History (31)
comment:1 Changed 5 years ago by
- Commit set to f08fb452108ef1c83eb9f0932d3014df44793350
comment:2 Changed 5 years ago by
- Status changed from new to needs_review
comment:3 in reply to: ↑ description ; follow-up: ↓ 9 Changed 4 years ago by
Replying to embray:
This one is slightly questionable in that there were many
cpdef
functions in this package that takechar *
arguments, and I changed them to take non-typed arguments (effectivelystr
).
They don't seem to be performance-critical methods, so I'm fine with it.
One potential problem for reviewing this ticket is that it involves several external packages, some of which are not even free to install.
comment:4 follow-up: ↓ 7 Changed 4 years ago by
This ticket makes me think of a general problem with str_to_bytes
: I think you should always accept bytes
for filenames (Python generally does that). So it might be a good idea to allow str_to_bytes
to accept bytes
input (I said this before but you refused that). What do you think?
comment:5 Changed 4 years ago by
- Status changed from needs_review to needs_work
In src/sage/numerical/backends/gurobi_backend.pyx
I see a if name
which should be if name is not None
comment:6 follow-up: ↓ 8 Changed 4 years ago by
Several "vertex" methods in glpk_graph_backend.pyx
used a take a char*
as argument. To me, it's not obvious to me that the canonical replacement is str
(as opposed to bytes
). In any case, it feels wrong to disallow bytes
.
comment:7 in reply to: ↑ 4 ; follow-up: ↓ 12 Changed 4 years ago by
Replying to jdemeyer:
This ticket makes me think of a general problem with
str_to_bytes
: I think you should always acceptbytes
for filenames (Python generally does that). So it might be a good idea to allowstr_to_bytes
to acceptbytes
input (I said this before but you refused that). What do you think?
That would actually be bad because it would break most cases which explicitly should not accept bytes. Specifically for filenames, it's better to just make a special case for accepting bytes. Though I don't think high-level file handling functions need accept bytes in most cases either unless there were an explicit reason to.
comment:8 in reply to: ↑ 6 Changed 4 years ago by
Replying to jdemeyer:
Several "vertex" methods in
glpk_graph_backend.pyx
used a take achar*
as argument. To me, it's not obvious to me that the canonical replacement isstr
(as opposed tobytes
). In any case, it feels wrong to disallowbytes
.
I can't say I'm 100% clear on that myself since it's not obvious that this code is even used anywhere in Sage. But it's duplicating the Sage Graph API and I don't see what makes you think it should accept bytes.
comment:9 in reply to: ↑ 3 Changed 4 years ago by
Replying to jdemeyer:
Replying to embray:
This one is slightly questionable in that there were many
cpdef
functions in this package that takechar *
arguments, and I changed them to take non-typed arguments (effectivelystr
).They don't seem to be performance-critical methods, so I'm fine with it.
One potential problem for reviewing this ticket is that it involves several external packages, some of which are not even free to install.
It is a problem, and some of these modules are not well tested because of it. Though in the cases I've changed it should be fairly clear that it's the right approach in general (i.e. we are passing python str
s to interfaces that expect char *
).
comment:10 Changed 4 years ago by
- Milestone changed from sage-8.2 to sage-8.3
comment:11 Changed 4 years ago by
- Milestone changed from sage-8.3 to sage-8.4
I believe this issue can reasonably be addressed for Sage 8.4.
comment:12 in reply to: ↑ 7 Changed 4 years ago by
Replying to embray:
Specifically for filenames, it's better to just make a special case for accepting bytes.
How about a new function filename_to_bytes()
? I never liked the look of str_to_bytes(fname, FS_ENCODING, 'surrogateescape')
anyway, filename_to_bytes(fname)
would look much better.
comment:13 Changed 4 years ago by
How does this ticket now relate to #26020? It wasn't clear to me why it needed to broken into a separate ticket in the first place.
Are we still going to add filename_to_bytes()
? I like the idea but it should probably be done separately.
comment:14 Changed 4 years ago by
- Status changed from needs_work to needs_info
comment:15 Changed 4 years ago by
These changes look fine to me (but I haven't tested anything).
comment:16 Changed 4 years ago by
- Milestone changed from sage-8.4 to sage-8.5
comment:17 Changed 4 years ago by
@embray, will you be able to finish this one and #24741 ?
comment:18 Changed 4 years ago by
Sorry, yeah, I'll take care of it. I think I stalled on this one because Jeroen had made some comments earlier, which seemed to suggest a course of action he preferred, and I was waiting for confirmation from him but it never came. I'll just clean up the existing fixes here and worry about the filename_to_bytes()
thing later.
comment:19 Changed 4 years ago by
- Status changed from needs_info to needs_work
comment:20 Changed 4 years ago by
- Commit changed from f08fb452108ef1c83eb9f0932d3014df44793350 to 3434d49f591123a2910bcdf83c8b65b3be5c78b4
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
3434d49 | py3: numerous string encode/decode fixes for sage.numerical.backends
|
comment:21 Changed 4 years ago by
- Status changed from needs_work to needs_review
I just rebased the existing branch on 8.5.beta6.
I can see that there's a case to be made for still allowing some of these methods to accept filenames as bytes, but I think this is fine for now until and unless a specific need for it arises.
comment:22 Changed 4 years ago by
- Status changed from needs_review to needs_work
the import of char_to_str
seem not to be used in backends/coin_backend.pyx
comment:23 Changed 4 years ago by
- Commit changed from 3434d49f591123a2910bcdf83c8b65b3be5c78b4 to da920d50745a997a176925c67130acd94abf87ee
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
da920d5 | py3: numerous string encode/decode fixes for sage.numerical.backends
|
comment:24 Changed 4 years ago by
- Status changed from needs_work to needs_review
comment:25 Changed 4 years ago by
- Reviewers set to Frédéric Chapoton
- Status changed from needs_review to needs_work
in src/sage/numerical/backends/cvxopt_sdp_backend.pyx, the INPUT field must be modified
same in src/sage/numerical/backends/generic_sdp_backend.pyx
otherwise, looks good to me (as far as I can tell)
comment:26 Changed 4 years ago by
Ah, very true (not that many people are likely to read that, but best to be consistent).
comment:27 Changed 4 years ago by
- Status changed from needs_work to needs_review
No, they're fine. I checked the code and it doesn't need updating in those cases. For some reason they already correctly read str
instead of char *
.
comment:28 Changed 4 years ago by
- Status changed from needs_review to positive_review
oh, well. Let it be.
@vbraun: probably safer to keep this one for 8.6.beta0
comment:29 Changed 4 years ago by
- Milestone changed from sage-8.5 to sage-8.6
comment:30 Changed 4 years ago by
- Branch changed from u/embray/python3/sage-numerical-backends/string to da920d50745a997a176925c67130acd94abf87ee
- Resolution set to fixed
- Status changed from positive_review to closed
comment:31 Changed 4 years ago by
- Commit da920d50745a997a176925c67130acd94abf87ee deleted
Maybe the problem described in ticket #26999 was produced by the numerous string fixes made here. Can you take a look?
Branch pushed to git repo; I updated commit sha1. New commits:
py3: numerous string encode/decode fixes for sage.numerical.backends