Opened 2 years ago
Closed 2 years ago
#25925 closed enhancement (fixed)
py3: Fix combinat.tiling module for python3
Reported by:  vklein  Owned by:  

Priority:  major  Milestone:  sage8.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
 Branch set to u/vklein/py3__fix_combinat_tiling_module_for_python3
comment:2 Changed 2 years ago by
 Commit set to f4fd3e228665c510a0b994b70340d1f8ab9e2926
 Status changed from new to needs_review
comment:3 Changed 2 years ago by
comment:4 Changed 2 years ago by
some failing doctest, see patchbot
comment:6 Changed 2 years ago by
 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
 Commit changed from f4fd3e228665c510a0b994b70340d1f8ab9e2926 to 138cc436e9c642fb733eef389e5d9e3bcd402a2c
Branch pushed to git repo; I updated commit sha1. New commits:
138cc43  Trac #25925: adapt quantumino long tests to tiling ...

comment:8 Changed 2 years ago by
 Keywords thursdaysbdx added
 Status changed from needs_work to needs_review
comment:9 Changed 2 years ago by
I looked the proposed changes. I have 4 comments:
 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 thenminxyz = vector(minxyz)
. Same formax
.
 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 callingnext
should be in thetryexcept
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)
 Replacing
G_set
(a set) byG
(a list) makes many things unefficient like calling__contains__
andremove
. Is there a way to avoid this?
 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
 Commit changed from 138cc436e9c642fb733eef389e5d9e3bcd402a2c to cee9783a131290c9dcf8337d9d4a1ebaa729063a
Branch pushed to git repo; I updated commit sha1. New commits:
cee9783  Trac #25925: many little modifications ...

comment:11 Changed 2 years ago by
Commit cee9783
Answer comments 1, 2 and 4.
comment:12 Changed 2 years ago by
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.
comment:13 Changed 2 years ago by
 Commit changed from cee9783a131290c9dcf8337d9d4a1ebaa729063a to c5963c1a8c5602c151178a6ced69dfa8936b77da
comment:14 Changed 2 years ago by
All comments fixed.
comment:15 Changed 2 years ago by
 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.
 What is this?
+from typing import List, Any + +
comment:16 Changed 2 years ago by
 documentation of
canonical_isometric_copies
should say that it returns alist
not aset
comment:17 followup: ↓ 19 Changed 2 years ago by
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
 Commit changed from c5963c1a8c5602c151178a6ced69dfa8936b77da to d531d542bc3977c1b0ab34d2a1aa517535c9d9df
Branch pushed to git repo; I updated commit sha1. New commits:
d531d54  Trac #25925: use a set instead of a list in ncube_isome ...

comment:19 in reply to: ↑ 17 Changed 2 years ago by
Replying to slabbe:
Thank you for your work on moving this module to Python 3.
You are welcome ! Last comments fixed.
comment:20 Changed 2 years ago by
 Status changed from needs_review to positive_review
Sorry for taking so long. I want to vacation before finishing the review.
comment:21 Changed 2 years ago by
 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
New commits:
Trac #25925: Fix tiling.py for python3.