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: |
Description (last modified by )
And general reorganization thereof.
Part of #12913.
Attachments (2)
Change History (18)
comment:2 Changed 10 years ago by
Dependencies: | → #12453 |
---|---|
Status: | new → needs_review |
The dependency on #12453 is due to doctest/sources.py
.
comment:3 Changed 9 years ago by
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
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
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.
comment:6 Changed 9 years ago by
Reviewers: | → Darij Grinberg |
---|
comment:7 Changed 9 years ago by
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
Attachment: | trac_14875-remove_cc_dyck_word-ts.patch added |
---|
comment:9 Changed 9 years ago by
Status: | needs_review → positive_review |
---|
comment:11 Changed 9 years ago by
Milestone: | sage-5.12 → sage-5.13 |
---|
comment:12 Changed 9 years ago by
Status: | positive_review → needs_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
Attachment: | trac_14875-remove_cc_dyck_word-ts-upd.patch added |
---|
updated version with edited doctest result on sources.py
comment:13 Changed 9 years ago by
That said, I have no idea what doctests aren't run in the first place... (and why).
comment:14 Changed 9 years ago by
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
Status: | needs_work → positive_review |
---|
Also thanks for making the quick change (I was just about to do it :P).
comment:16 Changed 9 years ago by
Merged in: | → sage-5.13.beta0 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
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.