Opened 6 years ago

Closed 12 months ago

Last modified 11 months ago

#16110 closed task (fixed)

Parallelogram Polyomino

Reported by: boussica Owned by: boussica
Priority: minor Milestone: sage-8.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) Commit: 41c18d9f8e7cebf1bd5f11181d22891151bb55cf
Dependencies: Stopgaps:

Description

Implementation of the Parallelogram Polyominoes

Change History (118)

comment:1 Changed 6 years ago by boussica

  • Branch set to u/boussica/parallelogram_polyomino

comment:2 Changed 6 years ago by git

  • Commit set to f32d04e18aa54d48311aeaadea91b7e3c6441a95

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

f32d04eAdd bijections to Dyck tableaux

comment:3 Changed 6 years ago by git

  • Commit changed from f32d04e18aa54d48311aeaadea91b7e3c6441a95 to a9a732dd1a0fc24b21d7d41f5c7f84f9430279b2

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

a9a732dImprove the parallelogram Polyominoes

comment:4 Changed 6 years ago by git

  • Commit changed from a9a732dd1a0fc24b21d7d41f5c7f84f9430279b2 to c256ca15dc6ce993b1227a44245dbb035d2c8af8

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

c256ca1Improve Parallelogram polyominoes

comment:5 Changed 6 years ago by boussica

  • Keywords days57 added

comment:6 Changed 6 years ago by git

  • Commit changed from c256ca15dc6ce993b1227a44245dbb035d2c8af8 to 3130e69ce63820990dc4dd0979884e5cdac8343f

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

3130e69Add some bijection to trees and improve the doc

comment:7 Changed 6 years ago by git

  • Commit changed from 3130e69ce63820990dc4dd0979884e5cdac8343f to 176197ea6ff2b556b3a3a2650101218756a54f9b

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

176197eAdd bij. OrderedTrees to parallelogramPolyominos

comment:8 Changed 6 years ago by git

  • Commit changed from 176197ea6ff2b556b3a3a2650101218756a54f9b to 773047af0af0ced5b42e122986faa49b3db83583

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

773047aAdd Combinatorail map in ParallelogramPolyomino

comment:9 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:10 Changed 5 years ago by chapoton

  • Branch changed from u/boussica/parallelogram_polyomino to u/chapoton/16110
  • Commit changed from 773047af0af0ced5b42e122986faa49b3db83583 to 600c74adb3a22d9125549263f8051f582a69d345
  • Dependencies set to #10194

New commits:

4ad6b34Merge branch 'u/boussica/parallelogram_polyomino' of ssh://trac.sagemath.org:22/sage into 16110
600c74atrac #16110 minor code formatting and doc details

comment:11 Changed 5 years ago by git

  • Commit changed from 600c74adb3a22d9125549263f8051f582a69d345 to ebd0bb4e6f11715619e90f3b1d3a8e27b4c8368e

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

ebd0bb4trac #16110 some typos corrected

comment:12 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:13 Changed 5 years ago by boussica

  • Branch changed from u/chapoton/16110 to u/boussica/16110

comment:14 Changed 5 years ago by git

  • Commit changed from ebd0bb4e6f11715619e90f3b1d3a8e27b4c8368e to b5e4192e55eee9fc5f6326303649b279ac73d024

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

b5e4192Add tests for special cases in PP.

comment:15 Changed 5 years ago by git

  • Commit changed from b5e4192e55eee9fc5f6326303649b279ac73d024 to e40248f11588826f0725b163ed2705df54779faa

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

e40248fImprove Parallelogram Polyomino drawing.

comment:16 Changed 5 years ago by git

  • Commit changed from e40248f11588826f0725b163ed2705df54779faa to 0a361e6df507b9e00db39f1dbc74e3d6b81bcabb

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

0a361e6Pep8 !

comment:17 Changed 5 years ago by git

  • Commit changed from 0a361e6df507b9e00db39f1dbc74e3d6b81bcabb to b7584cb8cc302a8b3d22764b77c83c82ce2549cd

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

b7584cbPep

comment:18 Changed 5 years ago by chapoton

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 4 years ago by chapoton

  • 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 4 years ago by chapoton

  • Milestone changed from sage-6.4 to sage-6.9

comment:21 Changed 4 years ago by chapoton

  • Status changed from new to needs_review

Well, I allow myself to put this into needs review..

comment:22 Changed 4 years ago by vdelecroix

  • 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 (non-strict) subclass of the metaclasses of all its bases

Vincent

comment:23 Changed 4 years ago by git

  • Commit changed from 9ca3937191df5debaab3dc200a5bbb301bb2e08c to af521d9e9558b0120853b7969415337ccc7741ab

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

7fd7fbaMerge branch 'u/chapoton/16110' into 6.9.b6
af521d9fixing metaclass

comment:24 Changed 4 years ago by chapoton

  • Dependencies changed from #10194 to #16204

New commits:

7fd7fbaMerge branch 'u/chapoton/16110' into 6.9.b6
af521d9fixing metaclass

comment:25 Changed 4 years ago by boussica

  • 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 4 years ago by boussica

  • Milestone changed from sage-6.9 to sage-6.10

comment:27 Changed 4 years ago by git

  • Commit changed from af6ea875ce064a3cbfa9995f9b99f63bc6aa325d to ea8b3d98e2afbcd7a77d1d42f909957288a30425

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

ea8b3d9Improve documentation

comment:28 Changed 4 years ago by git

  • Commit changed from ea8b3d98e2afbcd7a77d1d42f909957288a30425 to 67c4693da6dad52bcdcd28ed6f82fc5e56d9c200

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

67c4693Improve parallelogram polyomino bijection + doc.

comment:29 Changed 4 years ago by git

  • Commit changed from 67c4693da6dad52bcdcd28ed6f82fc5e56d9c200 to 5ffad37c02207231ed5f0dd481ec6095de793e18

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

feca1f0Improve documentation
5ffad37Some documentation

comment:30 Changed 4 years ago by git

  • Commit changed from 5ffad37c02207231ed5f0dd481ec6095de793e18 to f4ec302e1eb49972d6cffe490653bc343d302ae1

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

96ddaaeImprove documentation
3957b90Remove get_node() from abstract tree
9ac66b2Merge branch 't/16204/add_some_methods_in_trees' into t/16110/parallelogram_polyomino_2
f4ec302Small bug

comment:31 Changed 3 years ago by git

  • Commit changed from f4ec302e1eb49972d6cffe490653bc343d302ae1 to 5a90eb986368d236d47f41821d55f97509a0c672

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

0eb710dtrac 16204 some more doc, plus code details
79dc6e6Add Parallelogram polyominoes
f52b822Improve documentation
107a220Improve parallelogram polyomino bijection + doc.
23751ddImprove documentation
667fe14Some documentation
1ff1156Improve documentation
170a6c0Small bug
425e97cSolve Documentation problems
5a90eb9Merge branch 'u/boussica/parallelogram_polyomino_2' of git://trac.sagemath.org/sage into t/16110/parallelogram_polyomino_2

comment:32 Changed 3 years ago by git

  • Commit changed from 5a90eb986368d236d47f41821d55f97509a0c672 to c7270a6e2e42d165fc9a6f1923b774dc70f9b322

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

c7270a6Add some documentation

comment:33 Changed 3 years ago by boussica

  • Milestone changed from sage-6.10 to sage-7.3

comment:34 Changed 3 years ago by git

  • Commit changed from c7270a6e2e42d165fc9a6f1923b774dc70f9b322 to 99e238f5524b3a21200c0eef7744d4b822235fb2

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

99e238fAdd Documentation and Tests for PP.

comment:35 Changed 3 years ago by git

  • Commit changed from 99e238f5524b3a21200c0eef7744d4b822235fb2 to 4c4fd494444dffc872d2a8757253e08b181d2cb6

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

4c4fd49Now PP have the good size : the half-permiter

comment:36 Changed 3 years ago by git

  • Commit changed from 4c4fd494444dffc872d2a8757253e08b181d2cb6 to 70cda6372f9190c8e0e7cb9330a74699d204bbc1

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

70cda63Add some documentations

comment:37 Changed 3 years ago by git

  • Commit changed from 70cda6372f9190c8e0e7cb9330a74699d204bbc1 to 0b995ab7249e88474cbc1738f9e0a2b1bff1c8a6

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

0b995abImprove documentation

comment:38 Changed 3 years ago by git

  • Commit changed from 0b995ab7249e88474cbc1738f9e0a2b1bff1c8a6 to ae4513e1bf9ce830fa06499942d8ed7449e972d0

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

ae4513eDocumentation improvement

comment:39 Changed 3 years ago by chapoton

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/sage-patchbot/local/lib/python2.7/site-packages/sage/combinat/parallelogram_polyomino.py:docstring of sage.combinat.parallelogram_polyomino.default_tikz_options:10: ERROR: Unexpected indentation.
+[dochtml] [combinat ] /home/worker/sage-patchbot/local/lib/python2.7/site-packages/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 3 years ago by chapoton

  • Branch changed from u/boussica/parallelogram_polyomino_2 to public/16110
  • Commit changed from ae4513e1bf9ce830fa06499942d8ed7449e972d0 to 192d35d78b55d7f7ef3adc2751e6952ca0c95382

I have made a refreshed branch. There still remains

1) to have full and clean documentation

2) to adapt to the new standard of global options introduced by #18555


New commits:

9cbd427Merge branch 'u/boussica/parallelogram_polyomino_2' in 7.3.rc0
192d35dtrac 16110 refresh the code (partially)

comment:41 Changed 3 years ago by git

  • Commit changed from 192d35d78b55d7f7ef3adc2751e6952ca0c95382 to da7931ef631b7621cad05f4fee9f6236a69ef542

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

da7931eMerge branch 'develop' into t/16110/public/16110

comment:42 Changed 3 years ago by git

  • Commit changed from da7931ef631b7621cad05f4fee9f6236a69ef542 to 58628829100dbeab69d600d1d68120a8afe3e039

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

5862882Update documentation and port for sage 7.5

comment:43 Changed 3 years ago by git

  • Commit changed from 58628829100dbeab69d600d1d68120a8afe3e039 to 91fb16500543b261b2add6fdc46b06c9e14470ba

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

91fb165Implementation of LocalOption

comment:44 Changed 3 years ago by patxiku

  • Keywords days79 added

comment:45 Changed 3 years ago by git

  • Commit changed from 91fb16500543b261b2add6fdc46b06c9e14470ba to 4b279244d4741eed43c4505431dd9b67da272489

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

4b27924Solve error when compiling documentation

comment:46 Changed 3 years ago by git

  • Commit changed from 4b279244d4741eed43c4505431dd9b67da272489 to faa4de2be0454646fc0637f9fb077c54f11caf8d

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

faa4de2Doc updates

comment:47 Changed 3 years ago by git

  • Commit changed from faa4de2be0454646fc0637f9fb077c54f11caf8d to 35edd8d73d2c7d670291b8ec017f02664a6c14c3

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

35edd8dUpdat documentation of Parallelogram polyominoes

comment:48 Changed 3 years ago by git

  • Commit changed from 35edd8d73d2c7d670291b8ec017f02664a6c14c3 to e0b331b1ead37e9839aacf352f878706d4c1fb6a

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

34e14b9Add some documentation
e06550fMerge branch 'public/16110' of git://trac.sagemath.org/sage into t/16110/public/16110
e0b331bMerge + failed test

comment:49 Changed 3 years ago by chapoton

you should not use xrange but range

you should use python3 syntax for print, namely print("stuff")

see patchbot report

comment:50 Changed 3 years ago by git

  • Commit changed from e0b331b1ead37e9839aacf352f878706d4c1fb6a to 4f0788de92c7921cb057e8568975e68b886a6f14

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

4f0788dAdded some documentation and tests

comment:51 Changed 3 years ago by git

  • Commit changed from 4f0788de92c7921cb057e8568975e68b886a6f14 to b74afb7d4e41f8e7f833ef3015f5483ef0d68bfc

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

8ad387eImprove documentation
b74afb7Merge branch 'public/16110' of git://trac.sagemath.org/sage into t/16110/public/16110

comment:52 Changed 3 years ago by git

  • Commit changed from b74afb7d4e41f8e7f833ef3015f5483ef0d68bfc to 08dc7900e776d34890a928f6c4f8af977adb634f

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

08dc790Corrected some tests.

comment:53 Changed 3 years ago by git

  • Commit changed from 08dc7900e776d34890a928f6c4f8af977adb634f to 8c7fd34dc0a7f447a6c583c3438214330007c509

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

8c7fd34Added some documentations

comment:54 Changed 3 years ago by chapoton

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 3 years ago by boussica

  • Milestone changed from sage-7.3 to sage-7.5

comment:56 Changed 3 years ago by git

  • Commit changed from 8c7fd34dc0a7f447a6c583c3438214330007c509 to 8ba9ddd381577da00665b749813d7286cbb0f54d

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

ae33d76Merge branch 'public/16110' in 7.5.b5
8ba9dddtrac 16110 cleanup of xrange, print, pep8

comment:57 Changed 3 years ago by git

  • Commit changed from 8ba9ddd381577da00665b749813d7286cbb0f54d to 473cf1df8b2649091da4489eb8374c5bf7fc6905

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

89b1f3dMerge branch 'develop' into t/16110/public/16110
473cf1dImprove documentation and rename some functions

comment:58 Changed 3 years ago by boussica

  • Milestone changed from sage-7.5 to sage-8.0

comment:59 Changed 3 years ago by chapoton

see patchbot report for many problems (doc does not build, among others)

comment:60 Changed 3 years ago by git

  • Commit changed from 473cf1df8b2649091da4489eb8374c5bf7fc6905 to 09a098b6d6bcd12e12cb2ce2289c15107ee0ef7f

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

09a098btrac 16110 fixing the docbuild issue

comment:61 Changed 3 years ago by git

  • Commit changed from 09a098b6d6bcd12e12cb2ce2289c15107ee0ef7f to 5d3b55ce5fd64cb8099b116b5f6b0705830a61af

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

5d3b55ctrac 16110 fixing metaclass for python3

comment:62 Changed 3 years ago by chapoton

  • Dependencies #16204 deleted

comment:63 Changed 3 years ago by chapoton

Not full-coverage:

+combinat/parallelogram_polyomino.py: 93.8% (90 of 96)

And also should be lazy-imported.

comment:64 follow-up: Changed 3 years ago by git

  • Commit changed from 5d3b55ce5fd64cb8099b116b5f6b0705830a61af to a926e0acf59726e886c532d729737fe4edf29524

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

a926e0atrac 16110 lazy import

comment:65 in reply to: ↑ 64 Changed 3 years ago by boussica

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 2 years ago by git

  • Commit changed from a926e0acf59726e886c532d729737fe4edf29524 to f327a362825e362771be3abbec2e09b319b71ed2

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

f327a36trac 16110 adding some # indirect doctest

comment:67 Changed 2 years ago by git

  • Commit changed from f327a362825e362771be3abbec2e09b319b71ed2 to effab847919474177e98a85d5ffffc31e1672f4c

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

effab84Add some bibiography, complete all remaining TODOS

comment:68 Changed 2 years ago by slabbe

Two small comments to start:

  1. I would change to_parallelogram_polyomino_Boussicault_Socci to _to_parallelogram_polyomino_Boussicault_Socci since it is available through the method to_parallelogram_polyomino.
  1. 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 follow-ups: Changed 2 years ago by slabbe

Some more comments (just looking at the code, no test done yet):

  1. _get_path_in_pair_of_tree_from_box similar methods do not have doctests.
  1. (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, 169-206, 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 [...]
  1. Out of curiosity, why does the class _polyomino_row: needs to be inside the class instead of simply in the module?
  1. 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'
Last edited 2 years ago by slabbe (previous) (diff)

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

  1. bounce and bounce_path methods have no INPUT block.
  1. Use one line instead of three in the example of area, bounce and bounce_path methods (other occurrences?):
sage: pp = ParallelogramPolyomino(
....:     [[0, 1], [1, 0]]
....: )
sage: pp.area()
1

sage: pp = ParallelogramPolyomino(
....:     [[1], [1]]
....: )
  1. bad REST formatting in method box_is_root (missing :: maybe)
  1. In from_dyck_word, add `` around the string 'Delest-Viennot'
  1. In from_dyck_word, it says the default is foo, but it is None.
  1. In get_node_position_from_box, mention the default value:
nb_crossed_nodes – a list containg just one integer.
  1. INPUT block is missing in is_k_directed.
  1. In to_binary_tree and to_dyck_word and to_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, ‘Aval-Boussicault’.
+- ``bijection`` -- string or ``None`` (default: ``None``) The name of bijection to use for 
+  the convertion. The possible value are ``None`` or ``'Aval-Boussicault'``.
  1. 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 an EXAMPLE:: block. Similarly with the string starting with This is the default TIKZ options. This option is used to configurate element of a drawing to allow TIKZ code generation..

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

  1. 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 2 years ago by slabbe

  1. (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]]
....: )
  1. Also, the Python convention says to add space here:
    -sage: pp=ParallelogramPolyomino(L)
    +sage: pp = ParallelogramPolyomino(L)
    

comment:73 Changed 2 years ago by slabbe

  1. sage: ParallelogramPolyominoes? should mention in the INPUT and EXAMPLES blocks the case size=None.

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

  1. This is strange:
sage: P = ParallelogramPolyominoes()
sage: P.is_finite()
True
  1. 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'
Last edited 2 years ago by slabbe (previous) (diff)

comment:75 Changed 2 years ago by slabbe

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 2 years ago by slabbe

  • Reviewers set to Sébastien Labbé

comment:77 Changed 2 years ago by slabbe

  • Keywords thursdaysbdx added

comment:78 in reply to: ↑ 74 Changed 2 years ago by boussica

Replying to slabbe:

  1. This is strange:
sage: P = ParallelogramPolyominoes()
sage: P.is_finite()
True
  1. 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.

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

comment:79 Changed 2 years ago by git

  • Commit changed from effab847919474177e98a85d5ffffc31e1672f4c to 4051fe77f1c9862910293dff43d6049d16caf632

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

4051fe7Add tests to obtain 100% of coverage.

comment:80 in reply to: ↑ 71 Changed 2 years ago by boussica

Replying to slabbe:

  1. 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.

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

comment:81 Changed 2 years ago by chapoton

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 2 years ago by git

  • Commit changed from 4051fe77f1c9862910293dff43d6049d16caf632 to c71a8633b413346167ded84460c542225fe564e6

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

0b1f3abMerge branch 'public/16110' in 8.1.rc2
c71a863trac 16110 fixing my own comments

comment:83 Changed 2 years ago by chapoton

  • Milestone changed from sage-8.0 to sage-8.2
  • Status changed from needs_work to needs_review

comment:84 Changed 2 years ago by chapoton

bot is now green, so that we can start discuss the code

comment:85 follow-up: Changed 2 years ago by 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 sage-develop/src/doc/en/reference/references/index.rst
  • I do not understand the docstring in lines 819-821:
        @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) and heights (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 be Determine whether the cell at the given position is inside the parallelogram polyomino.
  • docstring of _polyomino_row could be This is an internal class representing a single row of a parallelogram polyomino.
  • diagramme in line 2434 should be diagram.

comment:86 Changed 2 years ago by git

  • Commit changed from c71a8633b413346167ded84460c542225fe564e6 to 6d528fcc96910f1a293c92a9288242831de753f0

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

1a0686dMerge branch 'public/16110' in 8.2.b2
6d528fctrac 16110 some details

comment:87 Changed 2 years ago by mantepse

  • 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 2 years ago by slabbe

  • 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 2 years ago by git

  • Commit changed from 6d528fcc96910f1a293c92a9288242831de753f0 to 8f9fe27df53d367ce04d232278636e9899dddf32

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

8f9fe27trac #16110 moved refs to main ref file

comment:90 Changed 20 months ago by mantepse

ping?

comment:91 Changed 20 months ago by slabbe

Adrien, I suggest you come at https://wiki.sagemath.org/thursdaysbdx this following Thursday and we finish this:)

comment:92 follow-up: Changed 18 months ago by mantepse

ping?

comment:93 in reply to: ↑ 92 Changed 14 months ago by boussica

Replying to mantepse:

ping?

pong, i will finish this task now. I have now some time to do this.

comment:94 Changed 14 months ago by mantepse

wonderful!

comment:95 Changed 14 months ago by git

  • Commit changed from 8f9fe27df53d367ce04d232278636e9899dddf32 to 9cb98d87bbfbd4a17384854b3990914e78601aab

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

79b037cMerge branch 't/16110/public/16110' into 8.4.b
9cb98d8Small correction in reference/references/index.rst

comment:96 Changed 14 months ago by mantepse

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 14 months ago by boussica

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):

  1. 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 14 months ago by boussica

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:

  1. 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:99 Changed 14 months ago by git

  • Commit changed from 9cb98d87bbfbd4a17384854b3990914e78601aab to 047ee01fedd467af71af7d895077c529cb5d250b

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

2966230Comment 68 - ticket 16110
779cafcAdd Input and Change some code formating
92c1be1Format some code in the doctest
0c70493See Review ticket 16110 comment 70 .10 .11. 12
047ee01Review ticket 16100 comment 70 .13 .14. 15

comment:100 in reply to: ↑ 70 Changed 14 months ago by boussica

I don't find what is the problem. Can you expand on that remark ?

Replying to slabbe:

  1. bad REST formatting in method box_is_root (missing :: maybe)

comment:101 Changed 14 months ago by git

  • Commit changed from 047ee01fedd467af71af7d895077c529cb5d250b to 74c229151a67dfd88771318193b385216b1d6b06

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

74c2291pep8 is the life ! Review Comment 72 .17

comment:102 Changed 13 months ago by git

  • Commit changed from 74c229151a67dfd88771318193b385216b1d6b06 to 37b4dbbc506ec39f5f14c7b615c94e5fddbe043f

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

a3c195bSolve problems raised by patchbot
37b4dbbImprove documentation of PPs

comment:103 Changed 13 months ago by git

  • Commit changed from 37b4dbbc506ec39f5f14c7b615c94e5fddbe043f to b56f871834e8bd9a03f2c3f97a91b0cfa06c6e36

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

e29cdbbMerge branch 't/16110/public/16110' into 8.5.b2
b56f871Review ticket 16100 comment 73 .19

comment:104 Changed 13 months ago by boussica

Review - Commment 71 .16 - coverage is done. Review - Comment 72 .18 - This is done by commit 74c2291

comment:105 Changed 13 months ago by git

  • 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 13 months ago by git

  • Commit changed from f11ba03c33f2c6f02e4411e88c26cf070e0f57f5 to bce053fba45b626618426f67a51a96307869b65b

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

bce053fPep 8

comment:107 in reply to: ↑ 85 Changed 13 months ago by boussica

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 sage-develop/src/doc/en/reference/references/index.rst
  • I do not understand the docstring in lines 819-821:
        @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) and heights (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 be Determine whether the cell at the given position is inside the parallelogram polyomino.
  • docstring of _polyomino_row could be This is an internal class representing a single row of a parallelogram polyomino.
  • diagramme in line 2434 should be diagram.

New commits:

f11ba03 Review ticket 16100 comment 85
bce053fPep 8

comment:108 Changed 13 months ago by git

  • Commit changed from bce053fba45b626618426f67a51a96307869b65b to 0cf531ca40302d1e3a11a45d78e862390fd1eb04

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

0cf531cImprove documentation

comment:109 Changed 13 months ago by git

  • Commit changed from 0cf531ca40302d1e3a11a45d78e862390fd1eb04 to 4b84af1bfc7ce8bbe827c1604de40470f0d8293f

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

4b84af1Merge branch 't/16110/public/16110' into 8.5.b3

comment:110 Changed 13 months ago by boussica

  • Milestone changed from sage-8.2 to sage-8.5
  • Status changed from needs_work to needs_review

comment:111 Changed 12 months ago by git

  • Commit changed from 4b84af1bfc7ce8bbe827c1604de40470f0d8293f to c079d3e9cd7fe8bfbf563bad4dc44c73650b8864

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

05aa2f0Merge branch 'public/16110' of trac.sagemath.org:sage into 16110
c079d3e16110: minor corrections during review

comment:112 follow-up: Changed 12 months ago by slabbe

  1. 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.
  1. 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
----------------------------------------------------------------------
  1. 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.
Last edited 12 months ago by slabbe (previous) (diff)

comment:113 Changed 12 months ago by slabbe

  • Status changed from needs_review to needs_work

comment:114 Changed 12 months ago by git

  • Commit changed from c079d3e9cd7fe8bfbf563bad4dc44c73650b8864 to 41c18d9f8e7cebf1bd5f11181d22891151bb55cf

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

4e7c7b0Merge branch 't/16110/public/16110' into 8.5.b6
41c18d9Trac #16110: Fix display order in __setitem__ and in ...

comment:115 in reply to: ↑ 112 Changed 12 months ago by vklein

Replying to slabbe:

  1. 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 12 months ago by slabbe

  • 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 12 months ago by vbraun

  • Branch changed from public/16110 to 41c18d9f8e7cebf1bd5f11181d22891151bb55cf
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:118 Changed 11 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.