Opened 19 months ago

Closed 9 months ago

Last modified 9 months ago

#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) 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 19 months ago by git

  • Commit set to f08fb452108ef1c83eb9f0932d3014df44793350

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

f08fb45py3: numerous string encode/decode fixes for sage.numerical.backends

comment:2 Changed 19 months ago by embray

  • Status changed from new to needs_review

comment:3 in reply to: ↑ description ; follow-up: Changed 19 months ago by jdemeyer

Replying to embray:

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).

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: Changed 19 months ago by jdemeyer

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 19 months ago by jdemeyer

  • 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: Changed 19 months ago by jdemeyer

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: Changed 19 months ago by embray

Replying to jdemeyer:

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?

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 19 months ago by embray

Replying to jdemeyer:

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.

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 19 months ago by embray

Replying to jdemeyer:

Replying to embray:

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).

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 strs to interfaces that expect char *).

comment:10 Changed 17 months ago by embray

  • Milestone changed from sage-8.2 to sage-8.3

comment:11 Changed 14 months ago by embray

  • 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 14 months ago by jdemeyer

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 13 months ago by embray

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 13 months ago by embray

  • Status changed from needs_work to needs_info

comment:15 Changed 12 months ago by mkoeppe

These changes look fine to me (but I haven't tested anything).

comment:16 Changed 11 months ago by embray

  • Milestone changed from sage-8.4 to sage-8.5

comment:17 Changed 10 months ago by chapoton

@embray, will you be able to finish this one and #24741 ?

comment:18 Changed 10 months ago by embray

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 10 months ago by chapoton

  • Status changed from needs_info to needs_work

comment:20 Changed 10 months ago by git

  • Commit changed from f08fb452108ef1c83eb9f0932d3014df44793350 to 3434d49f591123a2910bcdf83c8b65b3be5c78b4

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

3434d49py3: numerous string encode/decode fixes for sage.numerical.backends

comment:21 Changed 10 months ago by embray

  • 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 9 months ago by chapoton

  • 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 9 months ago by git

  • Commit changed from 3434d49f591123a2910bcdf83c8b65b3be5c78b4 to da920d50745a997a176925c67130acd94abf87ee

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

da920d5py3: numerous string encode/decode fixes for sage.numerical.backends

comment:24 Changed 9 months ago by embray

  • Status changed from needs_work to needs_review

comment:25 Changed 9 months ago by chapoton

  • 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 9 months ago by embray

Ah, very true (not that many people are likely to read that, but best to be consistent).

comment:27 Changed 9 months ago by embray

  • 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 9 months ago by chapoton

  • 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 9 months ago by chapoton

  • Milestone changed from sage-8.5 to sage-8.6

comment:30 Changed 9 months ago by vbraun

  • 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 9 months ago by slabbe

  • Commit da920d50745a997a176925c67130acd94abf87ee deleted

Maybe the problem described in ticket #26999 was produced by the numerous string fixes made here.

Version 0, edited 9 months ago by slabbe (next)
Note: See TracTickets for help on using tickets.