Opened 10 years ago

Closed 9 years ago

#14875 closed enhancement (fixed)

Remove CombinatorialClass from DyckWords

Reported by: Travis Scrimshaw Owned by: Sage Combinat CC user
Priority: major Milestone: sage-5.13
Component: combinatorics Keywords:
Cc: Sage Combinat CC user, Nicolas M. Thiéry Merged in: sage-5.13.beta0
Authors: Travis Scrimshaw Reviewers: Darij Grinberg
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Travis Scrimshaw)

And general reorganization thereof.

Part of #12913.

Apply: trac_14875-remove_cc_dyck_word-ts-upd.patch

Attachments (2)

trac_14875-remove_cc_dyck_word-ts.patch (126.4 KB) - added by Travis Scrimshaw 9 years ago.
trac_14875-remove_cc_dyck_word-ts-upd.patch (126.4 KB) - added by Darij Grinberg 9 years ago.
updated version with edited doctest result on sources.py

Download all attachments as: .zip

Change History (18)

comment:1 Changed 10 years ago by Darij Grinberg

Use the bijection [Knu1973]_ to send ``self`` to a `321`-avoiding
permutation.

This reference (to TAOCP III without any further pointer) might not be very helpful. It should specify pp. 60-61. Better, add a reference to 3.1 of http://arxiv.org/abs/0805.1325 which defines it explicitly.

Last edited 10 years ago by Darij Grinberg (previous) (diff)

comment:2 Changed 10 years ago by Travis Scrimshaw

Dependencies: #12453
Status: newneeds_review

The dependency on #12453 is due to doctest/sources.py.

comment:3 Changed 9 years ago by Darij Grinberg

Uploading some modifications, yet far from an actual review. One thing I'm doing is eliminating the dependency on the order of multiplication of permutations.

I don't understand what the aliases here are for (copypasta?):

    display=dict(default="list",
                 description='Specifies how Dyck words should be printed',
                 values=dict(list='displayed as a list',
                             lattice='displayed on the lattice defined by ``diagram_style``'),
                 alias=dict(exp="exp_low", compact="compact_low", array="diagram",
                           ferrers_diagram="diagram", young_diagram="diagram"),
                 case_sensitive=False),

comment:4 Changed 9 years ago by Travis Scrimshaw

Hey Darij,

That whole latex option is a copy/paste (copypasta) error and should be removed since there are (currently) no other latex output types.

Also if we review this before #12453, I can easily commute the patches.

Thanks,
Travis

comment:5 Changed 9 years ago by Darij Grinberg

Uploaded my review patch. Please look over it as I'm not 100% sure I always caught your correct intentions.

I took the freedom to remove def is_a_prefix (and replace all reference to it by ones to is_a), since it was doing precisely the same thing as def is_a. This might theoretically have caused some issues in other places, but at least the sage/combinat/*.py files doctest correctly (I have only done ./sage -bt devel/sage-main/sage/combinat; for doctesting the whole library I don't have the time right now).

Why did you remove the an_element() doctests?

Yes, please commute this ante #12453, as I don't feel I have very much to say about Cython structs.

Last edited 9 years ago by Darij Grinberg (previous) (diff)

comment:6 Changed 9 years ago by Darij Grinberg

Reviewers: Darij Grinberg

comment:7 Changed 9 years ago by Travis Scrimshaw

Dependencies: #12453

Hey Darij,

Thanks for doing the full review. I commuted it past #12453, folded in your review patch so I could make one small change of making is_a_prefix a deprecated function alias of is_a.

Thanks,
Travis

Changed 9 years ago by Travis Scrimshaw

comment:8 Changed 9 years ago by Travis Scrimshaw

For patchbot:

Apply: trac_14875-remove_cc_dyck_word-ts.patch

comment:9 Changed 9 years ago by Darij Grinberg

Status: needs_reviewpositive_review

comment:10 Changed 9 years ago by Travis Scrimshaw

Thank you again Darij.

comment:11 Changed 9 years ago by Jeroen Demeyer

Milestone: sage-5.12sage-5.13

comment:12 Changed 9 years ago by Jeroen Demeyer

Status: positive_reviewneeds_work
**********************************************************************
File "devel/sage/sage/doctest/sources.py", line 666, in sage.doctest.sources.FileDocTestSource._test_enough_doctests
Failed example:
    for path, dirs, files in itertools.chain(os.walk('sage'), os.walk('doc')): # long time
        path = os.path.relpath(path)
        dirs.sort(); files.sort()
        for F in files:
            _, ext = os.path.splitext(F)
            if ext in ('.py', '.pyx', '.sage', '.spyx', '.rst'):
                filename = os.path.join(path, F)
                FDS = FileDocTestSource(filename, DocTestDefaults(long=True,optional=True))
                FDS._test_enough_doctests(verbose=False)
Expected:
    There are 6 tests in sage/combinat/dyck_word.py that are not being run
    There are 18 tests in sage/combinat/partition.py that are not being run
    There are 15 tests in sage/combinat/permutation.py that are not being run
    There are 14 tests in sage/combinat/skew_partition.py that are not being run
    There are 18 tests in sage/combinat/tableau.py that are not being run
    There are 8 tests in sage/combinat/crystals/tensor_product.py that are not being run
    There are 15 tests in sage/combinat/root_system/cartan_type.py that are not being run
    There are 8 tests in sage/combinat/root_system/type_A.py that are not being run
    There are 8 tests in sage/combinat/root_system/type_G.py that are not being run
    There are 3 unexpected tests being run in sage/doctest/parsing.py
    There are 1 unexpected tests being run in sage/doctest/reporting.py
    There are 9 tests in sage/graphs/graph_plot.py that are not being run
    There are 2 tests in sage/server/notebook/worksheet.py that are not being run
Got:
    There are 7 tests in sage/combinat/dyck_word.py that are not being run
    There are 18 tests in sage/combinat/partition.py that are not being run
    There are 15 tests in sage/combinat/permutation.py that are not being run
    There are 14 tests in sage/combinat/skew_partition.py that are not being run
    There are 18 tests in sage/combinat/tableau.py that are not being run
    There are 8 tests in sage/combinat/crystals/tensor_product.py that are not being run
    There are 15 tests in sage/combinat/root_system/cartan_type.py that are not being run
    There are 8 tests in sage/combinat/root_system/type_A.py that are not being run
    There are 8 tests in sage/combinat/root_system/type_G.py that are not being run
    There are 3 unexpected tests being run in sage/doctest/parsing.py
    There are 1 unexpected tests being run in sage/doctest/reporting.py
    There are 9 tests in sage/graphs/graph_plot.py that are not being run
    There are 2 tests in sage/server/notebook/worksheet.py that are not being run
**********************************************************************

Changed 9 years ago by Darij Grinberg

updated version with edited doctest result on sources.py

comment:13 Changed 9 years ago by Darij Grinberg

That said, I have no idea what doctests aren't run in the first place... (and why).

comment:14 Changed 9 years ago by Travis Scrimshaw

Description: modified (diff)

It's because they are in dynamic docstrings (i.e. set via a __doc__ attribute), not static to a class or function. This is a known issue #14272.

For patchbot:

Apply: trac_14875-remove_cc_dyck_word-ts-upd.patch ​

comment:15 Changed 9 years ago by Travis Scrimshaw

Status: needs_workpositive_review

Also thanks for making the quick change (I was just about to do it :P).

comment:16 Changed 9 years ago by Jeroen Demeyer

Merged in: sage-5.13.beta0
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.