#16110 closed task (fixed)
Parallelogram Polyomino
Reported by:  boussica  Owned by:  boussica 

Priority:  minor  Milestone:  sage8.6 
Component:  combinatorics  Keywords:  Parallelogram polyomino, days57, days79, thursdaysbdx 
Cc:  Merged in:  
Authors:  Adrien Boussicault  Reviewers:  Sébastien Labbé, Martin Rubey 
Report Upstream:  N/A  Work issues:  
Branch:  41c18d9 (Commits, GitHub, GitLab)  Commit:  41c18d9f8e7cebf1bd5f11181d22891151bb55cf 
Dependencies:  Stopgaps: 
Description
Implementation of the Parallelogram Polyominoes
Change History (118)
comment:1 Changed 8 years ago by
 Branch set to u/boussica/parallelogram_polyomino
comment:2 Changed 8 years ago by
 Commit set to f32d04e18aa54d48311aeaadea91b7e3c6441a95
comment:3 Changed 8 years ago by
 Commit changed from f32d04e18aa54d48311aeaadea91b7e3c6441a95 to a9a732dd1a0fc24b21d7d41f5c7f84f9430279b2
Branch pushed to git repo; I updated commit sha1. New commits:
a9a732d  Improve the parallelogram Polyominoes

comment:4 Changed 8 years ago by
 Commit changed from a9a732dd1a0fc24b21d7d41f5c7f84f9430279b2 to c256ca15dc6ce993b1227a44245dbb035d2c8af8
Branch pushed to git repo; I updated commit sha1. New commits:
c256ca1  Improve Parallelogram polyominoes

comment:5 Changed 8 years ago by
 Keywords days57 added
comment:6 Changed 8 years ago by
 Commit changed from c256ca15dc6ce993b1227a44245dbb035d2c8af8 to 3130e69ce63820990dc4dd0979884e5cdac8343f
Branch pushed to git repo; I updated commit sha1. New commits:
3130e69  Add some bijection to trees and improve the doc

comment:7 Changed 8 years ago by
 Commit changed from 3130e69ce63820990dc4dd0979884e5cdac8343f to 176197ea6ff2b556b3a3a2650101218756a54f9b
Branch pushed to git repo; I updated commit sha1. New commits:
176197e  Add bij. OrderedTrees to parallelogramPolyominos

comment:8 Changed 8 years ago by
 Commit changed from 176197ea6ff2b556b3a3a2650101218756a54f9b to 773047af0af0ced5b42e122986faa49b3db83583
Branch pushed to git repo; I updated commit sha1. New commits:
773047a  Add Combinatorail map in ParallelogramPolyomino

comment:9 Changed 8 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:10 Changed 8 years ago by
 Branch changed from u/boussica/parallelogram_polyomino to u/chapoton/16110
 Commit changed from 773047af0af0ced5b42e122986faa49b3db83583 to 600c74adb3a22d9125549263f8051f582a69d345
 Dependencies set to #10194
comment:11 Changed 8 years ago by
 Commit changed from 600c74adb3a22d9125549263f8051f582a69d345 to ebd0bb4e6f11715619e90f3b1d3a8e27b4c8368e
Branch pushed to git repo; I updated commit sha1. New commits:
ebd0bb4  trac #16110 some typos corrected

comment:12 Changed 8 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:13 Changed 8 years ago by
 Branch changed from u/chapoton/16110 to u/boussica/16110
comment:14 Changed 8 years ago by
 Commit changed from ebd0bb4e6f11715619e90f3b1d3a8e27b4c8368e to b5e4192e55eee9fc5f6326303649b279ac73d024
Branch pushed to git repo; I updated commit sha1. New commits:
b5e4192  Add tests for special cases in PP.

comment:15 Changed 8 years ago by
 Commit changed from b5e4192e55eee9fc5f6326303649b279ac73d024 to e40248f11588826f0725b163ed2705df54779faa
Branch pushed to git repo; I updated commit sha1. New commits:
e40248f  Improve Parallelogram Polyomino drawing.

comment:16 Changed 8 years ago by
 Commit changed from e40248f11588826f0725b163ed2705df54779faa to 0a361e6df507b9e00db39f1dbc74e3d6b81bcabb
Branch pushed to git repo; I updated commit sha1. New commits:
0a361e6  Pep8 !

comment:17 Changed 8 years ago by
 Commit changed from 0a361e6df507b9e00db39f1dbc74e3d6b81bcabb to b7584cb8cc302a8b3d22764b77c83c82ce2549cd
Branch pushed to git repo; I updated commit sha1. New commits:
b7584cb  Pep

comment:18 Changed 7 years ago by
I have made a standalone branch available as u/chapoton/16110
Of course, it still depends on set factories, but not in the branch.
comment:19 Changed 7 years ago by
 Branch changed from u/boussica/16110 to u/chapoton/16110
 Commit changed from b7584cb8cc302a8b3d22764b77c83c82ce2549cd to 9ca3937191df5debaab3dc200a5bbb301bb2e08c
here is a branch that applies, your was not applying anymore.
Adrien, is this "needs review" ?
comment:20 Changed 7 years ago by
 Milestone changed from sage6.4 to sage6.9
comment:21 Changed 7 years ago by
 Status changed from new to needs_review
Well, I allow myself to put this into needs review..
comment:22 Changed 7 years ago by
 Status changed from needs_review to needs_work
Bonjour à tous,
Sage does not start with the branch applied.
> 173 class ParallelogramPolyomino(ClonableList): global ParallelogramPolyomino = undefined global ClonableList = None 174 r""" 175 The class of Parallelogram Polyominoes 176 """ 177 __metaclass__ = ClasscallMetaclass ... TypeError: Error when calling the metaclass bases metaclass conflict: the metaclass of a derived class must be a (nonstrict) subclass of the metaclasses of all its bases
Vincent
comment:23 Changed 7 years ago by
 Commit changed from 9ca3937191df5debaab3dc200a5bbb301bb2e08c to af521d9e9558b0120853b7969415337ccc7741ab
comment:24 Changed 7 years ago by
 Dependencies changed from #10194 to #16204
comment:25 Changed 7 years ago by
 Branch changed from u/chapoton/16110 to u/boussica/parallelogram_polyomino_2
 Commit changed from af521d9e9558b0120853b7969415337ccc7741ab to af6ea875ce064a3cbfa9995f9b99f63bc6aa325d
To solve some merge/rebase difficulties. I create a new branch with all the code present in the branch (e467677f92400e46cb27142e0623e693aa5db2ac).
I add the parallelogram polyominoe in the documentation.
comment:26 Changed 7 years ago by
 Milestone changed from sage6.9 to sage6.10
comment:27 Changed 7 years ago by
 Commit changed from af6ea875ce064a3cbfa9995f9b99f63bc6aa325d to ea8b3d98e2afbcd7a77d1d42f909957288a30425
Branch pushed to git repo; I updated commit sha1. New commits:
ea8b3d9  Improve documentation

comment:28 Changed 7 years ago by
 Commit changed from ea8b3d98e2afbcd7a77d1d42f909957288a30425 to 67c4693da6dad52bcdcd28ed6f82fc5e56d9c200
Branch pushed to git repo; I updated commit sha1. New commits:
67c4693  Improve parallelogram polyomino bijection + doc.

comment:29 Changed 7 years ago by
 Commit changed from 67c4693da6dad52bcdcd28ed6f82fc5e56d9c200 to 5ffad37c02207231ed5f0dd481ec6095de793e18
comment:30 Changed 7 years ago by
 Commit changed from 5ffad37c02207231ed5f0dd481ec6095de793e18 to f4ec302e1eb49972d6cffe490653bc343d302ae1
comment:31 Changed 6 years ago by
 Commit changed from f4ec302e1eb49972d6cffe490653bc343d302ae1 to 5a90eb986368d236d47f41821d55f97509a0c672
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
0eb710d  trac 16204 some more doc, plus code details

79dc6e6  Add Parallelogram polyominoes

f52b822  Improve documentation

107a220  Improve parallelogram polyomino bijection + doc.

23751dd  Improve documentation

667fe14  Some documentation

1ff1156  Improve documentation

170a6c0  Small bug

425e97c  Solve Documentation problems

5a90eb9  Merge branch 'u/boussica/parallelogram_polyomino_2' of git://trac.sagemath.org/sage into t/16110/parallelogram_polyomino_2

comment:32 Changed 6 years ago by
 Commit changed from 5a90eb986368d236d47f41821d55f97509a0c672 to c7270a6e2e42d165fc9a6f1923b774dc70f9b322
Branch pushed to git repo; I updated commit sha1. New commits:
c7270a6  Add some documentation

comment:33 Changed 6 years ago by
 Milestone changed from sage6.10 to sage7.3
comment:34 Changed 6 years ago by
 Commit changed from c7270a6e2e42d165fc9a6f1923b774dc70f9b322 to 99e238f5524b3a21200c0eef7744d4b822235fb2
Branch pushed to git repo; I updated commit sha1. New commits:
99e238f  Add Documentation and Tests for PP.

comment:35 Changed 6 years ago by
 Commit changed from 99e238f5524b3a21200c0eef7744d4b822235fb2 to 4c4fd494444dffc872d2a8757253e08b181d2cb6
Branch pushed to git repo; I updated commit sha1. New commits:
4c4fd49  Now PP have the good size : the halfpermiter

comment:36 Changed 6 years ago by
 Commit changed from 4c4fd494444dffc872d2a8757253e08b181d2cb6 to 70cda6372f9190c8e0e7cb9330a74699d204bbc1
Branch pushed to git repo; I updated commit sha1. New commits:
70cda63  Add some documentations

comment:37 Changed 6 years ago by
 Commit changed from 70cda6372f9190c8e0e7cb9330a74699d204bbc1 to 0b995ab7249e88474cbc1738f9e0a2b1bff1c8a6
Branch pushed to git repo; I updated commit sha1. New commits:
0b995ab  Improve documentation

comment:38 Changed 6 years ago by
 Commit changed from 0b995ab7249e88474cbc1738f9e0a2b1bff1c8a6 to ae4513e1bf9ce830fa06499942d8ed7449e972d0
Branch pushed to git repo; I updated commit sha1. New commits:
ae4513e  Documentation improvement

comment:39 Changed 6 years ago by
Still need more doc, every function must be documented:
combinat/parallelogram_polyomino.py: 80.9% (72 of 89)
Some error in doc formatting:
+[dochtml] [combinat ] /home/worker/sagepatchbot/local/lib/python2.7/sitepackages/sage/combinat/parallelogram_polyomino.py:docstring of sage.combinat.parallelogram_polyomino.default_tikz_options:10: ERROR: Unexpected indentation. +[dochtml] [combinat ] /home/worker/sagepatchbot/local/lib/python2.7/sitepackages/sage/combinat/parallelogram_polyomino.py:docstring of sage.combinat.parallelogram_polyomino.default_tikz_options:15: ERROR: Unexpected indentation.
The function "breadth_node_paths_generator" has been given another name in #16204. So doctests here do not pass.
comment:40 Changed 6 years ago by
 Branch changed from u/boussica/parallelogram_polyomino_2 to public/16110
 Commit changed from ae4513e1bf9ce830fa06499942d8ed7449e972d0 to 192d35d78b55d7f7ef3adc2751e6952ca0c95382
comment:41 Changed 6 years ago by
 Commit changed from 192d35d78b55d7f7ef3adc2751e6952ca0c95382 to da7931ef631b7621cad05f4fee9f6236a69ef542
Branch pushed to git repo; I updated commit sha1. New commits:
da7931e  Merge branch 'develop' into t/16110/public/16110

comment:42 Changed 6 years ago by
 Commit changed from da7931ef631b7621cad05f4fee9f6236a69ef542 to 58628829100dbeab69d600d1d68120a8afe3e039
Branch pushed to git repo; I updated commit sha1. New commits:
5862882  Update documentation and port for sage 7.5

comment:43 Changed 6 years ago by
 Commit changed from 58628829100dbeab69d600d1d68120a8afe3e039 to 91fb16500543b261b2add6fdc46b06c9e14470ba
Branch pushed to git repo; I updated commit sha1. New commits:
91fb165  Implementation of LocalOption

comment:44 Changed 6 years ago by
 Keywords days79 added
comment:45 Changed 6 years ago by
 Commit changed from 91fb16500543b261b2add6fdc46b06c9e14470ba to 4b279244d4741eed43c4505431dd9b67da272489
Branch pushed to git repo; I updated commit sha1. New commits:
4b27924  Solve error when compiling documentation

comment:46 Changed 6 years ago by
 Commit changed from 4b279244d4741eed43c4505431dd9b67da272489 to faa4de2be0454646fc0637f9fb077c54f11caf8d
Branch pushed to git repo; I updated commit sha1. New commits:
faa4de2  Doc updates

comment:47 Changed 6 years ago by
 Commit changed from faa4de2be0454646fc0637f9fb077c54f11caf8d to 35edd8d73d2c7d670291b8ec017f02664a6c14c3
Branch pushed to git repo; I updated commit sha1. New commits:
35edd8d  Updat documentation of Parallelogram polyominoes

comment:48 Changed 6 years ago by
 Commit changed from 35edd8d73d2c7d670291b8ec017f02664a6c14c3 to e0b331b1ead37e9839aacf352f878706d4c1fb6a
comment:49 Changed 6 years ago by
you should not use xrange but range
you should use python3 syntax for print, namely print("stuff")
see patchbot report
comment:50 Changed 6 years ago by
 Commit changed from e0b331b1ead37e9839aacf352f878706d4c1fb6a to 4f0788de92c7921cb057e8568975e68b886a6f14
Branch pushed to git repo; I updated commit sha1. New commits:
4f0788d  Added some documentation and tests

comment:51 Changed 6 years ago by
 Commit changed from 4f0788de92c7921cb057e8568975e68b886a6f14 to b74afb7d4e41f8e7f833ef3015f5483ef0d68bfc
comment:52 Changed 6 years ago by
 Commit changed from b74afb7d4e41f8e7f833ef3015f5483ef0d68bfc to 08dc7900e776d34890a928f6c4f8af977adb634f
Branch pushed to git repo; I updated commit sha1. New commits:
08dc790  Corrected some tests.

comment:53 Changed 6 years ago by
 Commit changed from 08dc7900e776d34890a928f6c4f8af977adb634f to 8c7fd34dc0a7f447a6c583c3438214330007c509
Branch pushed to git repo; I updated commit sha1. New commits:
8c7fd34  Added some documentations

comment:54 Changed 6 years ago by
once again:
you should not use xrange but range
you should use python3 syntax for print, namely print("stuff")
see patchbot report
comment:55 Changed 6 years ago by
 Milestone changed from sage7.3 to sage7.5
comment:56 Changed 6 years ago by
 Commit changed from 8c7fd34dc0a7f447a6c583c3438214330007c509 to 8ba9ddd381577da00665b749813d7286cbb0f54d
comment:57 Changed 5 years ago by
 Commit changed from 8ba9ddd381577da00665b749813d7286cbb0f54d to 473cf1df8b2649091da4489eb8374c5bf7fc6905
comment:58 Changed 5 years ago by
 Milestone changed from sage7.5 to sage8.0
comment:59 Changed 5 years ago by
see patchbot report for many problems (doc does not build, among others)
comment:60 Changed 5 years ago by
 Commit changed from 473cf1df8b2649091da4489eb8374c5bf7fc6905 to 09a098b6d6bcd12e12cb2ce2289c15107ee0ef7f
Branch pushed to git repo; I updated commit sha1. New commits:
09a098b  trac 16110 fixing the docbuild issue

comment:61 Changed 5 years ago by
 Commit changed from 09a098b6d6bcd12e12cb2ce2289c15107ee0ef7f to 5d3b55ce5fd64cb8099b116b5f6b0705830a61af
Branch pushed to git repo; I updated commit sha1. New commits:
5d3b55c  trac 16110 fixing metaclass for python3

comment:62 Changed 5 years ago by
 Dependencies #16204 deleted
comment:63 Changed 5 years ago by
Not fullcoverage:
+combinat/parallelogram_polyomino.py: 93.8% (90 of 96)
And also should be lazyimported.
comment:64 followup: ↓ 65 Changed 5 years ago by
 Commit changed from 5d3b55ce5fd64cb8099b116b5f6b0705830a61af to a926e0acf59726e886c532d729737fe4edf29524
Branch pushed to git repo; I updated commit sha1. New commits:
a926e0a  trac 16110 lazy import

comment:65 in reply to: ↑ 64 Changed 5 years ago by
Thank you, i will work on that Ticket Thursday. We have, now, a working group : https://wiki.sagemath.org/thursdaysbdx.
I think, i will finish this ticket thursday.
comment:66 Changed 5 years ago by
 Commit changed from a926e0acf59726e886c532d729737fe4edf29524 to f327a362825e362771be3abbec2e09b319b71ed2
Branch pushed to git repo; I updated commit sha1. New commits:
f327a36  trac 16110 adding some # indirect doctest

comment:67 Changed 5 years ago by
 Commit changed from f327a362825e362771be3abbec2e09b319b71ed2 to effab847919474177e98a85d5ffffc31e1672f4c
Branch pushed to git repo; I updated commit sha1. New commits:
effab84  Add some bibiography, complete all remaining TODOS

comment:68 Changed 5 years ago by
Two small comments to start:
 I would change
to_parallelogram_polyomino_Boussicault_Socci
to_to_parallelogram_polyomino_Boussicault_Socci
since it is available through the methodto_parallelogram_polyomino
.
 I suggest to support Python 3 syntax as most as possible for any new module to get into sage for example by adding the following line at the top of the module:
# Support of Python 3 from __future__ import division, absolute_import, print_function, unicode_literals
comment:69 followups: ↓ 97 ↓ 98 Changed 5 years ago by
Some more comments (just looking at the code, no test done yet):
_get_path_in_pair_of_tree_from_box
similar methods do not have doctests.
 (many occurrences) References format do not follow the conventions:
+ By default, we use the bijection from the following article : + Ref. Delest, M.P. and Viennot, G. + "Algebraic Languages and Polyominoes Enumeration." + Theoret. Comput. Sci. 34, 169206, 1984. + (the bijection is described in the previous article on page 179 + and 180. An example is given in Figure 6)
See this page of the developper's manual:
All bibliographical references should be stored in the master bibliography file, SAGE_ROOT/src/doc/en/reference/references/index.rst, in the format [...]
 Out of curiosity, why does the
class _polyomino_row:
needs to be inside the class instead of simply in the module?
 The class
class _drawing_tool: r""" Technical class to produce TIKZ drawing.
is of larger interest than just the parallelogram polyominoes (I often need such thing). But I think it is anormal to
sage: from sage.combinat.parallelogram_polyomino import _drawing_tool
I think you should rather create a new module:
sage: from sage.misc.tikz import DrawingTool
I have the idea of geting my module TikzPicture into sage in the future. The class DrawingTool
and TikzPicture
could go in the same module. I let you create that module first into Sage.
For example if you install my package with sage pip install slabbe
, you may do:
sage: L = [[0,0,0,1,1,0,1,0,0,1,1,1],[1,1,1,0,0,1,1,0,0,1,0,0]] sage: pp = ParallelogramPolyomino(L) sage: from slabbe import TikzPicture sage: TikzPicture(latex(pp)).pdf() '[...]/62669/tikz_mZXN3H.pdf'
comment:70 followup: ↓ 100 Changed 5 years ago by
bounce
andbounce_path
methods have noINPUT
block.
 Use one line instead of three in the example of
area
,bounce
andbounce_path
methods (other occurrences?):
sage: pp = ParallelogramPolyomino( ....: [[0, 1], [1, 0]] ....: ) sage: pp.area() 1 sage: pp = ParallelogramPolyomino( ....: [[1], [1]] ....: )
 bad REST formatting in method
box_is_root
(missing :: maybe)
 In
from_dyck_word
, add``
around the string'DelestViennot'
 In
from_dyck_word
, it says the default is foo, but it isNone
.
 In
get_node_position_from_box
, mention the default value:
nb_crossed_nodes – a list containg just one integer.
 INPUT block is missing in
is_k_directed
.
 In
to_binary_tree
andto_dyck_word
andto_ordered_tree
, documentation does not follow the convention:
!diff  ``bijection`` – None (default) The name of bijection to use for  the convertion. The possible value are, ‘AvalBoussicault’. + ``bijection``  string or ``None`` (default: ``None``) The name of bijection to use for + the convertion. The possible value are ``None`` or ``'AvalBoussicault'``.
 The string starting with
This global option contains all the data needed by the Parallelogram classes to draw, display in ASCII, compile in latex a parallelogram polyomino.
should contain anEXAMPLE::
block. Similarly with the string starting withThis is the default TIKZ options. This option is used to configurate element of a drawing to allow TIKZ code generation.
.
comment:71 followup: ↓ 80 Changed 5 years ago by
 coverage is still not 100%:
$ sage coverage src/sage/combinat/parallelogram_polyomino.py  SCORE src/sage/combinat/parallelogram_polyomino.py: 93.8% (90 of 96)
comment:72 Changed 5 years ago by
 (this is a preference of myself, you may prefer to ignore it if you want). I prefer
this
sage: L = [[0,0,0,1,1,0,1,0,0,1,1,1],[1,1,1,0,0,1,1,0,0,1,0,0]] sage: pp=ParallelogramPolyomino(L)
or
sage: A = [0,0,0,1,1,0,1,0,0,1,1,1] sage: B = [1,1,1,0,0,1,1,0,0,1,0,0] sage: pp=ParallelogramPolyomino([A, B])
to
sage: pp=ParallelogramPolyomino( ....: [[0,0,0,1,1,0,1,0,0,1,1,1],[1,1,1,0,0,1,1,0,0,1,0,0]] ....: )
 Also, the Python convention says to add space here:
sage: pp=ParallelogramPolyomino(L) +sage: pp = ParallelogramPolyomino(L)
comment:73 Changed 5 years ago by
sage: ParallelogramPolyominoes?
should mention in the INPUT and EXAMPLES blocks the casesize=None
.
comment:74 followup: ↓ 78 Changed 5 years ago by
 This is strange:
sage: P = ParallelogramPolyominoes() sage: P.is_finite() True
 This does not work (but maybe it is okay to not work):
sage: P = ParallelogramPolyominoes() sage: len(list(P.some_elements())) # this works 100 sage: P.random_element() ... TypeError: int() argument must be a string or a number, not 'PlusInfinity'
comment:75 Changed 5 years ago by
Changing the status to needs work (oups, it was already!).
Other than that, I get
sage t long src/sage/combinat/parallelogram_polyomino.py [484 tests, 3.09 s]  All tests passed!
Tell me when it needs a next round of review.
comment:76 Changed 5 years ago by
 Reviewers set to Sébastien Labbé
comment:77 Changed 5 years ago by
 Keywords thursdaysbdx added
comment:78 in reply to: ↑ 74 Changed 5 years ago by
Replying to slabbe:
 This is strange:
sage: P = ParallelogramPolyominoes() sage: P.is_finite() True
 This does not work (but maybe it is okay to not work):
sage: P = ParallelogramPolyominoes() sage: len(list(P.some_elements())) # this works 100 sage: P.random_element() ... TypeError: int() argument must be a string or a number, not 'PlusInfinity'
In think it is a bug of set_factories. This example show the problem :
sage: from sage.structure.set_factories import * sage: from sage.structure.set_factories_example import * sage: XYPairs() AllPairs sage: APS = XYPairs() sage: APS.is_finite() True
I will report that bug.
comment:79 Changed 5 years ago by
 Commit changed from effab847919474177e98a85d5ffffc31e1672f4c to 4051fe77f1c9862910293dff43d6049d16caf632
Branch pushed to git repo; I updated commit sha1. New commits:
4051fe7  Add tests to obtain 100% of coverage.

comment:80 in reply to: ↑ 71 Changed 5 years ago by
Replying to slabbe:
 coverage is still not 100%:
$ sage coverage src/sage/combinat/parallelogram_polyomino.py  SCORE src/sage/combinat/parallelogram_polyomino.py: 93.8% (90 of 96)
Now we have 100% of coverage.
In ParallelogramPolyominoesFactory?, I have commented the function add_constraint() that become nonImplemented, because i don't use them in the set_factory. I don't know if it is a good way to solve the problem, but I don't use that function, so, I don't know how to test it.
comment:81 Changed 5 years ago by
This is not correct :
+ EXAMPLES:: + + sage: PP=ParallelogramPolyomino
because you must indent the test lines by 4 spaces.
This is not correct :
+ EXAMPLES:: + sage: pp = ParallelogramPolyomino(
because you must have an empty line after EXAMPLES::
This is not a correct way to add a doctest :
Return a default policy. + + sage: ParallelogramPolyominoes._default_policy
because you have forgotten EXAMPLES::
You can just remove "add_constraints(self, cons, args_opts):", I think.
comment:82 Changed 5 years ago by
 Commit changed from 4051fe77f1c9862910293dff43d6049d16caf632 to c71a8633b413346167ded84460c542225fe564e6
comment:83 Changed 5 years ago by
 Milestone changed from sage8.0 to sage8.2
 Status changed from needs_work to needs_review
comment:84 Changed 5 years ago by
bot is now green, so that we can start discuss the code
comment:85 followup: ↓ 107 Changed 5 years ago by
I would like to see this merged! Here are a few comments concerning docstrings:
 the error messages in
check
need spell and semantic checking :)
 the references should be moved to
sagedevelop/src/doc/en/reference/references/index.rst
 I do not understand the docstring in lines 819821:
@staticmethod def __classcall_private__(cls, *args, **opts): r""" Ensure that parallelogram polyominoes created by the enumerated sets and directly are the same and that they are instances of :class:`ParallelogramPolyomino`.
 docstrings of
widths
(line 1702) andheights
(line 1843) should begin with imperative.
 concerning
val
(line 1976): this looks like a useful function, why not make it public? It's docstring should beDetermine whether the cell at the given position is inside the parallelogram polyomino.
 docstring of
_polyomino_row
could beThis is an internal class representing a single row of a parallelogram polyomino.
diagramme
in line 2434 should bediagram
.
comment:86 Changed 5 years ago by
 Commit changed from c71a8633b413346167ded84460c542225fe564e6 to 6d528fcc96910f1a293c92a9288242831de753f0
comment:87 Changed 5 years ago by
 Reviewers changed from Sébastien Labbé to Sébastien Labbé, Martin Rubey
Sébastien, can we merge this? You can set it to positive review on my behalf, in case you are happy with it.
comment:88 Changed 5 years ago by
 Status changed from needs_review to needs_work
Well, not all of the 21 comments I gave have been fixed. And I think all of them do not take much long to fix.
comment:89 Changed 5 years ago by
 Commit changed from 6d528fcc96910f1a293c92a9288242831de753f0 to 8f9fe27df53d367ce04d232278636e9899dddf32
Branch pushed to git repo; I updated commit sha1. New commits:
8f9fe27  trac #16110 moved refs to main ref file

comment:90 Changed 4 years ago by
ping?
comment:91 Changed 4 years ago by
Adrien, I suggest you come at https://wiki.sagemath.org/thursdaysbdx this following Thursday and we finish this:)
comment:92 followup: ↓ 93 Changed 4 years ago by
ping?
comment:93 in reply to: ↑ 92 Changed 4 years ago by
comment:94 Changed 4 years ago by
wonderful!
comment:95 Changed 4 years ago by
 Commit changed from 8f9fe27df53d367ce04d232278636e9899dddf32 to 9cb98d87bbfbd4a17384854b3990914e78601aab
comment:96 Changed 4 years ago by
Les p be the bounce path of the parallelogram polyomino (:meth:`bounce_path`).
should be
Let `p` be the bounce path of the parallelogram polyomino (:meth:`bounce_path`).
def _repr_(self): r""" Return the string representation of the set of parallelogram polyominoes
should be
def _repr_(self): r""" Return the string representation of the set of parallelogram polyominoes.
comment:97 in reply to: ↑ 69 Changed 4 years ago by
There is no particular reason. Just to highlight that this class is a small tool dedicated to ParallelogramPolyominoe?.
Replying to slabbe:
Some more comments (just looking at the code, no test done yet):
 Out of curiosity, why does the
class _polyomino_row:
needs to be inside the class instead of simply in the module?
comment:98 in reply to: ↑ 69 Changed 4 years ago by
Merging All Drawing Tools coming from you tikz package and the drawing tools of ParallelogramPolyomino? is a big work :
 we need to list all the needed functionality
 we need to merge the ParallelogramPolyomino? codes and the code of your package
 we need to integrate the result on sage
 we need to make tests and documentation
I think its better to open a new ticket in order to close this one. We will treat that new ticket after this one.
I opened that new ticket : ticket #26508
Replying to slabbe:
 The class
class _drawing_tool: r""" Technical class to produce TIKZ drawing.is of larger interest than just the parallelogram polyominoes (I often need such thing). But I think it is anormal to
sage: from sage.combinat.parallelogram_polyomino import _drawing_toolI think you should rather create a new module:
sage: from sage.misc.tikz import DrawingToolI have the idea of geting my module TikzPicture into sage in the future. The class
DrawingTool
andTikzPicture
could go in the same module. I let you create that module first into Sage.For example if you install my package with
sage pip install slabbe
, you may do:sage: L = [[0,0,0,1,1,0,1,0,0,1,1,1],[1,1,1,0,0,1,1,0,0,1,0,0]] sage: pp = ParallelogramPolyomino(L) sage: from slabbe import TikzPicture sage: TikzPicture(latex(pp)).pdf() '[...]/62669/tikz_mZXN3H.pdf'
comment:99 Changed 4 years ago by
 Commit changed from 9cb98d87bbfbd4a17384854b3990914e78601aab to 047ee01fedd467af71af7d895077c529cb5d250b
comment:100 in reply to: ↑ 70 Changed 4 years ago by
I don't find what is the problem. Can you expand on that remark ?
Replying to slabbe:
 bad REST formatting in method
box_is_root
(missing :: maybe)
comment:101 Changed 4 years ago by
 Commit changed from 047ee01fedd467af71af7d895077c529cb5d250b to 74c229151a67dfd88771318193b385216b1d6b06
Branch pushed to git repo; I updated commit sha1. New commits:
74c2291  pep8 is the life ! Review Comment 72 .17

comment:102 Changed 4 years ago by
 Commit changed from 74c229151a67dfd88771318193b385216b1d6b06 to 37b4dbbc506ec39f5f14c7b615c94e5fddbe043f
comment:103 Changed 4 years ago by
 Commit changed from 37b4dbbc506ec39f5f14c7b615c94e5fddbe043f to b56f871834e8bd9a03f2c3f97a91b0cfa06c6e36
comment:104 Changed 4 years ago by
Review  Commment 71 .16  coverage is done. Review  Comment 72 .18  This is done by commit 74c2291
comment:105 Changed 4 years ago by
 Commit changed from b56f871834e8bd9a03f2c3f97a91b0cfa06c6e36 to f11ba03c33f2c6f02e4411e88c26cf070e0f57f5
Branch pushed to git repo; I updated commit sha1. New commits:
f11ba03  Review ticket 16100 comment 85

comment:106 Changed 4 years ago by
 Commit changed from f11ba03c33f2c6f02e4411e88c26cf070e0f57f5 to bce053fba45b626618426f67a51a96307869b65b
Branch pushed to git repo; I updated commit sha1. New commits:
bce053f  Pep 8

comment:107 in reply to: ↑ 85 Changed 4 years ago by
All the corrections of comment 85 was done.
The function classcall_private needs to be implemented to work with SetFactory?. With that function, the parent method will work correctly.
Replying to mantepse:
I would like to see this merged! Here are a few comments concerning docstrings:
 the error messages in
check
need spell and semantic checking :)
 the references should be moved to
sagedevelop/src/doc/en/reference/references/index.rst
 I do not understand the docstring in lines 819821:
@staticmethod def __classcall_private__(cls, *args, **opts): r""" Ensure that parallelogram polyominoes created by the enumerated sets and directly are the same and that they are instances of :class:`ParallelogramPolyomino`.
 docstrings of
widths
(line 1702) andheights
(line 1843) should begin with imperative.
 concerning
val
(line 1976): this looks like a useful function, why not make it public? It's docstring should beDetermine whether the cell at the given position is inside the parallelogram polyomino.
 docstring of
_polyomino_row
could beThis is an internal class representing a single row of a parallelogram polyomino.
diagramme
in line 2434 should bediagram
.
New commits:
f11ba03  Review ticket 16100 comment 85

bce053f  Pep 8

comment:108 Changed 4 years ago by
 Commit changed from bce053fba45b626618426f67a51a96307869b65b to 0cf531ca40302d1e3a11a45d78e862390fd1eb04
Branch pushed to git repo; I updated commit sha1. New commits:
0cf531c  Improve documentation

comment:109 Changed 4 years ago by
 Commit changed from 0cf531ca40302d1e3a11a45d78e862390fd1eb04 to 4b84af1bfc7ce8bbe827c1604de40470f0d8293f
Branch pushed to git repo; I updated commit sha1. New commits:
4b84af1  Merge branch 't/16110/public/16110' into 8.5.b3

comment:110 Changed 4 years ago by
 Milestone changed from sage8.2 to sage8.5
 Status changed from needs_work to needs_review
comment:111 Changed 4 years ago by
 Commit changed from 4b84af1bfc7ce8bbe827c1604de40470f0d8293f to c079d3e9cd7fe8bfbf563bad4dc44c73650b8864
comment:112 followup: ↓ 115 Changed 4 years ago by
 I just reviewed the code. Looks good to me. I did some trivial modifications while I was looking at it. I allowed myself to push a commit on top of the public branch.
 Using Sage with Python 3, I get 3 failures (see below). So I change the status to needs work because of that:
sage t long src/sage/combinat/parallelogram_polyomino.py ********************************************************************** File "src/sage/combinat/parallelogram_polyomino.py", line 238, in sage.combinat.parallelogram_polyomino.LocalOptions.__setitem__ Failed example: o["display"]="?" Expected: Current value : diagram {'default': 'list', 'values': {'diagram': 'diagram representation', 'list': 'list representation'}} Got: Current value : diagram {'default': 'list', 'values': {'list': 'list representation', 'diagram': 'diagram representation'}} ********************************************************************** File "src/sage/combinat/parallelogram_polyomino.py", line 304, in sage.combinat.parallelogram_polyomino.LocalOptions.__call__ Failed example: o(display="?") Expected: Current value : diagram {'default': 'list', 'values': {'diagram': 'diagram representation', 'list': 'list representation'}} Got: Current value : diagram {'default': 'list', 'values': {'list': 'list representation', 'diagram': 'diagram representation'}} ********************************************************************** File "src/sage/combinat/parallelogram_polyomino.py", line 1538, in sage.combinat.parallelogram_polyomino.ParallelogramPolyomino.get_options Failed example: pp.get_options() Expected: Current options for ParallelogramPolyominoes_size  display: list  drawing_components: {'diagram': True, 'bounce_1': False, 'tree': False, 'bounce_0': False}  latex: drawing  tikz_options: {'color_bounce_1': u'blue', 'color_bounce_0': u'red', 'point_size': 3.5, 'line_size': 1, 'color_line': u'black', 'color_point': u'black', 'scale': 1, 'mirror': None, 'rotation': 0, 'translation': [0, 0]} Got: Current options for ParallelogramPolyominoes_size  display: list  drawing_components: {'diagram': True, 'tree': False, 'bounce_0': False, 'bounce_1': False}  latex: drawing  tikz_options: {'scale': 1, 'line_size': 1, 'point_size': 3.5, 'color_line': 'black', 'color_point': 'black', 'color_bounce_0': 'red', 'color_bounce_1': 'blue', 'translation': [0, 0], 'rotation': 0, 'mirror': None} ********************************************************************** 3 items had failures: 1 of 7 in sage.combinat.parallelogram_polyomino.LocalOptions.__call__ 1 of 11 in sage.combinat.parallelogram_polyomino.LocalOptions.__setitem__ 1 of 3 in sage.combinat.parallelogram_polyomino.ParallelogramPolyomino.get_options [554 tests, 3 failures, 0.26 s]  sage t long src/sage/combinat/parallelogram_polyomino.py # 3 doctests failed 
 The branch now contains more than 70 commits including lots of merge with the most recent develop branch of Sage. I would suggest to rebase the branch on top of the most recent develop version of sage so that the history tree do not get to dirty because of this ticket. You may try
git rebase i HEAD~3
to rebase the last 3 commits for instance (this is usually what I do). Of course, rebasing the branch will change the hash of all involved commits. So this is okay only if nobody have code depending on that branch.
comment:113 Changed 4 years ago by
 Status changed from needs_review to needs_work
comment:114 Changed 4 years ago by
 Commit changed from c079d3e9cd7fe8bfbf563bad4dc44c73650b8864 to 41c18d9f8e7cebf1bd5f11181d22891151bb55cf
comment:115 in reply to: ↑ 112 Changed 4 years ago by
Replying to slabbe:
 Using Sage with Python 3, I get 3 failures (see below). So I change the status to needs work because of that:
Python3 errors fixed
comment:116 Changed 4 years ago by
 Status changed from needs_work to positive_review
I now get
sage t long src/sage/combinat/parallelogram_polyomino.py [554 tests, 0.28 s]  All tests passed! 
both in Python 2 and 3. Documentation builds fine. Positive review.
comment:117 Changed 4 years ago by
 Branch changed from public/16110 to 41c18d9f8e7cebf1bd5f11181d22891151bb55cf
 Resolution set to fixed
 Status changed from positive_review to closed
comment:118 Changed 4 years ago by
 Milestone changed from sage8.5 to sage8.6
This tickets were closed as fixed after the Sage 8.5 release.
Branch pushed to git repo; I updated commit sha1. New commits:
Add bijections to Dyck tableaux