Opened 4 years ago

Closed 3 years ago

# IncidenceStructure breaks if points cannot be sorted

Reported by: Owned by: jdemeyer major sage-8.7 combinatorics vklein Jeroen Demeyer Frédéric Chapoton N/A a354dc1 a354dc129b0085acf88b4b1cbae1d5029218fa61

This sort can fail if the points are not comparable:

sortkey = lambda e: [(0 if x is None else 2 if isinstance(x, str) else 1, x) for x in e]\
if isinstance(e, tuple) else e
self._points = sorted(points, key=sortkey)

The sortkey tries to find a clever way to "sort" the points but hardcoding classes like that is not a general solution. In particular, it's still comparing integers and strings when #22029 is appplied.

### comment:1 Changed 4 years ago by jdemeyer

• Description modified (diff)

### comment:2 Changed 4 years ago by jdemeyer

• Description modified (diff)

### comment:3 Changed 4 years ago by jdemeyer

• Description modified (diff)

### comment:4 follow-up: ↓ 6 Changed 4 years ago by vklein

A mix of points of strings type and integers type will not make this function fail.

This work in python3:

>>> (1, 3) < (2, 'a string')
True

The comparison stops if the first element of each tuple are different.

### comment:5 Changed 4 years ago by vklein

Also, the sortkey should be implemented as ordinary function def sortkey instead of this unreadable one-liner (why do people feel that a lambda is required to define a sorting key?).

Indeed, pep8 says so, my bad.

### comment:6 in reply to: ↑ 4 Changed 4 years ago by jdemeyer

A mix of points of strings type and integers type will not make this function fail.

Apparently, it does. See the patchbot report on #22029. I have not looked at the details.

### comment:7 Changed 4 years ago by jdemeyer

On #22029, I do get

TypeError: unsupported operand parent(s) for >: 'Integer Ring' and '<type 'str'>'

and it's pointing to the line

self._points = sorted(points, key=sortkey)

### comment:8 Changed 4 years ago by jdemeyer

• Branch set to u/jdemeyer/incidencestructure_breaks_if_points_cannot_be_sorted

### comment:9 Changed 4 years ago by jdemeyer

• Authors set to Jeroen Demeyer
• Commit set to a354dc129b0085acf88b4b1cbae1d5029218fa61
• Description modified (diff)
• Status changed from new to needs_review

New commits:

 ​a354dc1 Do not sort points in IncidenceStructure

### comment:10 Changed 4 years ago by jdemeyer

• Description modified (diff)

### comment:11 Changed 3 years ago by chapoton

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

ok, let it be

### comment:12 Changed 3 years ago by embray

• Milestone changed from sage-8.6 to sage-8.7

Retarging tickets optimistically to the next milestone. If you are responsible for this ticket (either its reporter or owner) and don't believe you are likely to complete this ticket before the next release (8.7) please retarget this ticket's milestone to sage-pending or sage-wishlist.

### comment:13 Changed 3 years ago by vbraun

• Branch changed from u/jdemeyer/incidencestructure_breaks_if_points_cannot_be_sorted to a354dc129b0085acf88b4b1cbae1d5029218fa61
• Resolution set to fixed
• Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.