Opened 2 years ago

Closed 22 months ago

#25925 closed enhancement (fixed)

py3: Fix combinat.tiling module for python3

Reported by: vklein Owned by:
Priority: major Milestone: sage-8.4
Component: python3 Keywords: thursdaysbdx
Cc: slabbe Merged in:
Authors: Vincent Klein Reviewers: Sébastien Labbé
Report Upstream: N/A Work issues:
Branch: d531d54 (Commits) Commit: d531d542bc3977c1b0ab34d2a1aa517535c9d9df
Dependencies: Stopgaps:

Description

Fix combinat.tiling module for python3.

Change History (21)

comment:1 Changed 2 years ago by vklein

  • Branch set to u/vklein/py3__fix_combinat_tiling_module_for_python3

comment:2 Changed 2 years ago by vklein

  • Authors set to Vincent Klein
  • Commit set to f4fd3e228665c510a0b994b70340d1f8ab9e2926
  • Status changed from new to needs_review

New commits:

f4fd3e2Trac #25925: Fix tiling.py for python3.

comment:3 Changed 2 years ago by vklein

  • Authors changed from Vincent Klein to Vincent Klein, Sébastien Labbé

comment:4 Changed 2 years ago by chapoton

some failing doctest, see patchbot

comment:5 Changed 2 years ago by vklein

  • Status changed from needs_review to needs_work

Indeed.

comment:6 Changed 2 years ago by slabbe

  • Authors changed from Vincent Klein, Sébastien Labbé to Vincent Klein
  • Reviewers set to Sébastien Labbé

Vincent and me discussed together this week about this ticket. I shared by thoughts with him and did some suggestions, but I will play the role of a reviewer for this ticket, not of an author.

comment:7 Changed 2 years ago by git

  • Commit changed from f4fd3e228665c510a0b994b70340d1f8ab9e2926 to 138cc436e9c642fb733eef389e5d9e3bcd402a2c

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

138cc43Trac #25925: adapt quantumino long tests to tiling ...

comment:8 Changed 2 years ago by vklein

  • Keywords thursdaysbdx added
  • Status changed from needs_work to needs_review

comment:9 Changed 2 years ago by slabbe

I looked the proposed changes. I have 4 comments:

  1. I do not like to use min and max as variable since this erases/conflicts with the Python function having the same name. I prefer you use minxyz and then minxyz = vector(minxyz). Same for max.
  1. The two new try are trying too much code and do not follow PEP 8 -- Style Guide for Python Code which I cite below. Only the line calling next should be in the try-except clause.
Additionally, for all try/except clauses, limit the try clause to the absolute minimum amount of code necessary. Again, this avoids masking bugs.

Yes:

try:
    value = collection[key]
except KeyError:
    return key_not_found(key)
else:
    return handle_value(value)

No:

try:
    # Too broad!
    return handle_value(collection[key])
except KeyError:
    # Will also catch KeyError raised by handle_value()
    return key_not_found(key)
  1. Replacing G_set (a set) by G (a list) makes many things unefficient like calling __contains__ and remove. Is there a way to avoid this?
  1. I do not like the change return rows -> return sorted(rows). Is it really necessary since the other changes make the list of matrix transformations unique?

comment:10 Changed 2 years ago by git

  • Commit changed from 138cc436e9c642fb733eef389e5d9e3bcd402a2c to cee9783a131290c9dcf8337d9d4a1ebaa729063a

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

cee9783Trac #25925: many little modifications ...

comment:11 Changed 2 years ago by vklein

Commit ​cee9783 Answer comments 1, 2 and 4.

comment:12 Changed 2 years ago by slabbe

In the quantumino file, I suggest to fix the lines

Polyomino: [(0, 0, 0), (1, 0, 0), (1, 1, 0), (1, 1, 1), (1, 2, 0)], Color: deeppink

as

Polyomino: [(...), (...), (...), (...), (...)], Color: ...

once and for all.

Last edited 2 years ago by slabbe (previous) (diff)

comment:13 Changed 2 years ago by git

  • Commit changed from cee9783a131290c9dcf8337d9d4a1ebaa729063a to c5963c1a8c5602c151178a6ced69dfa8936b77da

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

5d0df80Trac #25925: Fix quantumino long doctests to be ...
c5963c1Trac #25925: ncube_isometry_group_cosets faster algorithm

comment:14 Changed 2 years ago by vklein

All comments fixed.

comment:15 Changed 2 years ago by slabbe

  1. The line
    assert all(h in G for h in H), "H must be a subset of G"
    

is not so efficient. I suggest you move earlier the creation of G_todo and that you use G_todo in this assert line.

  1. What is this?
    +from typing import List, Any
    +
    +
    

comment:16 Changed 2 years ago by slabbe

  1. documentation of canonical_isometric_copies should say that it returns a list not a set

comment:17 follow-up: Changed 2 years ago by slabbe

I have no other comments for now. I will finish the review next week.

Thank you for your work on moving this module to Python 3.

comment:18 Changed 2 years ago by git

  • Commit changed from c5963c1a8c5602c151178a6ced69dfa8936b77da to d531d542bc3977c1b0ab34d2a1aa517535c9d9df

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

d531d54Trac #25925: use a set instead of a list in ncube_isome ...

comment:19 in reply to: ↑ 17 Changed 2 years ago by vklein

Replying to slabbe:

Thank you for your work on moving this module to Python 3.

You are welcome ! Last comments fixed.

comment:20 Changed 22 months ago by slabbe

  • Status changed from needs_review to positive_review

Sorry for taking so long. I want to vacation before finishing the review.

comment:21 Changed 22 months ago by vbraun

  • Branch changed from u/vklein/py3__fix_combinat_tiling_module_for_python3 to d531d542bc3977c1b0ab34d2a1aa517535c9d9df
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.