Opened 10 months ago

Closed 9 months ago

Last modified 9 months ago

#26870 closed enhancement (fixed)

py3: fix error with map in strongly_regular_db.pyx

Reported by: dcoudert Owned by:
Priority: major Milestone: sage-8.6
Component: graph theory Keywords: py3, graph
Cc: tscrim, chapoton, embray Merged in:
Authors: David Coudert Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: ee621e8 (Commits) Commit: ee621e8daf4c88e27e0a186abb9d3118b8d01eab
Dependencies: Stopgaps:

Description

map is an iterator in Python3, so some care is needed.

  • in strongly_regular_db.pyx, the graph constructor was called with a set of vertices defined by a map. We ensure that we build the list of vertices first.
  • same for the call to method IntersectionGraph.

We also fix some issues with bytes and str.

Change History (20)

comment:1 Changed 10 months ago by dcoudert

  • Branch set to public/26870_strongly_regular_db
  • Commit set to a5c862b039dce115a5bfa2eafec03b4975e912f9
  • Status changed from new to needs_review

New commits:

a5c862btrac #26870: fix error with map

comment:2 Changed 10 months ago by chapoton

  • Cc embray added

This looks very suspicious:

bytes_to_str(brouwer_data['comments'].encode('ascii', 'ignore')))

Probably there should be a shorter way..

comment:3 Changed 10 months ago by embray

Please don't do list(map(...)). Instead convert these to list comprehensions or generator expressions as appropriate.

comment:4 Changed 10 months ago by git

  • Commit changed from a5c862b039dce115a5bfa2eafec03b4975e912f9 to df223f97f93bc6d3abb3878a2c4161785b7770a9

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

33c75cdtrac #26870: use list comprehension
df223f9trac #26870: not using bytes_to_str

comment:5 Changed 10 months ago by dcoudert

This passes doctests with Python3 but not with Python2. There is an encoding issue as the database contains Östergård.

What should I do ?

comment:6 Changed 10 months ago by chapoton

Maybe try adding encoding='utf-8' inside the open(..) in cdef load_brouwer_database() ?

comment:7 Changed 10 months ago by dcoudert

open(...) does not accept the encoding parameter in Python2 (TypeError: 'encoding' is an invalid keyword argument for this function). It works with Python3.

I also tried to put encoding='utf-8' in json.load(...) but it does not solve the issue in Python2 :(

comment:8 Changed 10 months ago by chapoton

using io.open() ?

comment:9 Changed 10 months ago by dcoudert

no, still get the same error with py2.

Failed example:
    graphs.strongly_regular_graph(324,57,0,12)
Expected:
    Traceback (most recent call last):
    ...
    EmptySetError: Andries Brouwer's database reports that no (324, 57, 0,
    12)-strongly regular graph exists. Comments: <a
    href="srgtabrefs.html#GavrilyukMakhnev05">Gavrilyuk & Makhnev</a> and <a
    href="srgtabrefs.html#KaskiOstergard07">Kaski & Östergård</a>
Got:
    <BLANKLINE>
    Traceback (most recent call last):
      File "/Users/dcoudert/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 671, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/Users/dcoudert/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 1086, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.graphs.strongly_regular_db.strongly_regular_graph[6]>", line 1, in <module>
        graphs.strongly_regular_graph(Integer(324),Integer(57),Integer(0),Integer(12))
      File "sage/graphs/strongly_regular_db.pyx", line 3002, in sage.graphs.strongly_regular_db.strongly_regular_graph (build/cythonized/sage/graphs/strongly_regular_db.c:42610)
        raise EmptySetError("Andries Brouwer's database reports that no " +
    EmptySetError: Andries Brouwer's database reports that no (324, 57, 0, 12)-strongly regular graph exists. Comments: <a href="srgtabrefs.html#GavrilyukMakhnev05">Gavrilyuk & Makhnev</a> and <a href="srgtabrefs.html#KaskiOstergard07">Kaski & \xd6sterg\xe5rd</a>

comment:10 Changed 10 months ago by git

  • Commit changed from df223f97f93bc6d3abb3878a2c4161785b7770a9 to a527cea57c00a6888286e525d4a6b98fff7bc861

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

9dc4d3aa bit of trivial cleanup for readability
a527ceaimprove formatting of these error messages using f-strings

comment:11 Changed 10 months ago by embray

Try this ^

The problem with the original code, on Python 2, was that it was interpolating unicode strings into templates that were non-unicode strings. On Python 2 this results in implicit encoding of the unicode strings as ASCII, which results in a UnicodeEncodeError for any comments that contain non-ASCII characters.

Would have been better if the original code just used unicode strings for the error messages in the first place. Now, since this is Cython code, we can use f-strings which are implicitly unicode anyways, and make the formatting look a bit nicer.

comment:12 Changed 10 months ago by dcoudert

It's way nicer. I didn't know f-strings which seems very convenient. However, I still get Kaski & \xd6sterg\xe5rd instead of Kaski & Östergård with py2 :(

comment:13 Changed 10 months ago by embray

Yikes, I didn't realize this myself (it has not come up often), but it turns out on Python 2 most exceptions can't accept exception messages as unicode objects (rather, it can, but it will still try to implicitly encode them with ASCII)! :( https://pythonhosted.org/kitchen/unicode-frustrations.html#frustration-5-exceptions

The best thing to do would be to convert them to bytes, but only on Python 2, just using UTF-8 probably.

Last edited 10 months ago by embray (previous) (diff)

comment:14 Changed 9 months ago by dcoudert

not sure how to do that...

comment:15 Changed 9 months ago by chapoton

I would just suggest to put ... at the appropriate place in the failing doctest. We do not really care about these details..

comment:16 Changed 9 months ago by git

  • Commit changed from a527cea57c00a6888286e525d4a6b98fff7bc861 to ee621e8daf4c88e27e0a186abb9d3118b8d01eab

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

ee621e8trac #26870: simplify doctest

comment:17 Changed 9 months ago by dcoudert

good idea.

comment:18 Changed 9 months ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

ok, good for me.

comment:19 Changed 9 months ago by vbraun

  • Branch changed from public/26870_strongly_regular_db to ee621e8daf4c88e27e0a186abb9d3118b8d01eab
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:20 Changed 9 months ago by embray

  • Milestone changed from sage-8.5 to sage-8.6

This tickets were closed as fixed after the Sage 8.5 release.

Note: See TracTickets for help on using tickets.