Opened 4 years ago

Closed 2 years ago

#25434 closed enhancement (fixed)

Path_Tableaux

Reported by: Bruce Owned by: Bruce
Priority: major Milestone: sage-9.2
Component: combinatorics Keywords: fpsac2019
Cc: sage-combinat, tscrim, mantepse, aschilling, deinst, kdilks Merged in:
Authors: Bruce Westbury Reviewers: Ben Salisbury, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 9d3a838 (Commits, GitHub, GitLab) Commit: 9d3a838478a8c7f218bf333c2a58f598c2fe7f8b
Dependencies: Stopgaps:

Status badges

Description (last modified by Bruce)

This is a project to implement an abstraction of rectification, evacuation and promotion. These are conventionally defined on standard tableaux using jeu-de-taquin. The file catalan.py gives a toy implementation. I intend to provide other implementations.

Change History (111)

comment:1 Changed 4 years ago by Bruce

I have been unable to push my commits. I get Permission Denied. I will try again in the morning.

comment:2 Changed 4 years ago by tscrim

  • Cc sage-combinat tscrim added
  • Component changed from PLEASE CHANGE to combinatorics
  • Type changed from PLEASE CHANGE to enhancement

comment:3 Changed 4 years ago by Bruce

  • Branch set to u/Bruce/pathtableaux

comment:4 Changed 4 years ago by Bruce

  • Commit set to b4a14f9782c9584fa79344e4902eb147810ebac5
  • Description modified (diff)
  • Owner changed from (none) to Bruce

comment:5 Changed 4 years ago by Bruce

  • Cc mantepse aschilling added

comment:6 Changed 4 years ago by embray

  • Branch u/Bruce/pathtableaux deleted
  • Commit b4a14f9782c9584fa79344e4902eb147810ebac5 deleted

It seems there was something about this branch causing the git merge plugin to break so I have deleted the branch for now and will investigate further later.

comment:7 follow-up: Changed 4 years ago by embray

It seems the branch was just the current develop with no new commits on it, so maybe something about that scenario broke the plugin.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 4 years ago by Bruce

Replying to embray:

It seems the branch was just the current develop with no new commits on it, so maybe something about that scenario broke the plugin.

Thanks for sorting that out. I struggled to follow the instructions and no doubt got it messed up.

comment:9 Changed 4 years ago by Bruce

  • Branch set to u/Bruce/path_tableaux

comment:10 Changed 4 years ago by gh-darijgr

  • Branch changed from u/Bruce/path_tableaux to u/Bruce/pathtableaux
  • Commit set to 3090a75ec766aa05f83ee6046cabed79b7abe369

New commits:

e09a7bbThis is the initial commit for the project on Path Tableaux
934c015Corrected path
7157b1fEdited a couple of doc tests
3090a75A few trivial changes.

comment:11 Changed 4 years ago by git

  • Commit changed from 3090a75ec766aa05f83ee6046cabed79b7abe369 to 8944c6e6c854148d2f884346663c305a7c6990f5

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

a89516eChanged all.py
8944c6eFixed imports so doc tests now run.

comment:12 Changed 4 years ago by git

  • Commit changed from 8944c6e6c854148d2f884346663c305a7c6990f5 to 7d6fef08c58ddce1e3dea7b52ff4081ead34c245

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

7d6fef0Imports into semistandard.py fixed

comment:13 in reply to: ↑ 8 Changed 4 years ago by embray

Replying to Bruce:

Replying to embray:

It seems the branch was just the current develop with no new commits on it, so maybe something about that scenario broke the plugin.

Thanks for sorting that out. I struggled to follow the instructions and no doubt got it messed up.

No problem, not your fault at all.

comment:14 Changed 4 years ago by git

  • Commit changed from 7d6fef08c58ddce1e3dea7b52ff4081ead34c245 to 391ffd0c447336a2459b2a87265bb246372bd71d

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

bfcfc84Added doc tests
391ffd0More doc testing

comment:15 Changed 4 years ago by git

  • Commit changed from 391ffd0c447336a2459b2a87265bb246372bd71d to 16c04c43e726fe41005c3346e672503a9702277d

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

f9abd6bStarted doc testing semistandard.py
0d1e91fAdded rectify and multiply
16c04c4Added rectify

comment:16 Changed 4 years ago by git

  • Commit changed from 16c04c43e726fe41005c3346e672503a9702277d to e5cb8617c06746325523d815a6c28728a225e4a1

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

e5cb861Rectify added

comment:17 Changed 4 years ago by Bruce

  • Authors set to Bruce Westbury
  • Status changed from new to needs_review

I now have the basic functionality in the two implementations I have committed. I would appreciate some feedback before I commit other implementations. I have attempted to explain the purpose of this project but no doubt I could do better.

comment:18 Changed 4 years ago by tscrim

  • Status changed from needs_review to needs_work

I strongly believe that PathTableaux should not be a category. You should just have an abstract base class (ABC) for the *Tableau objects (the ElementMethods is currently playing this role, but you're using a sledgehammer in place of a ball-peen). It is something that is easy to change back if there becomes a practical use/need for the category, but I think it puts more distance between the concrete class and the ABC code, which makes it harder to maintain.

At this point, I am thinking it might be best to make a new folder in the combinat folder called tableau with your new code (one file for the ABC and your other two files). Then it will become the eventual home for all tableau files (but moving those should be done on separate tickets).

Some other misc comments:

  • What is this doing here: #!/usr/bin/env python2?
  • I would call the method plot instead of draw.
  • Your drawL and drawC might be best as separate standalone functions in the ABC file. Otherwise if you feel that they should be methods, then make them hidden by calling them _drawL and _drawC.
  • I don't see the point of having draw_partition in drawL as an internal helper function (in drawC, it makes sense because of the two for loops and avoiding code duplication).
  • There are a bunch of malformatted docstrings (EXAMPLES: -> EXAMPLES:: (or missing altogether) when followed by code, which should be indented by 1), and some that should be at the class level or at the top of the module.
  • The copyright should be comments, not part of the docstring.

comment:19 Changed 4 years ago by git

  • Commit changed from e5cb8617c06746325523d815a6c28728a225e4a1 to 0ae66fe9d4bdf4c90b3ccc1211baa692f66a3311

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

0ae66feCorrected formatting of doctests

comment:20 Changed 4 years ago by Bruce

Hi Travis, Thank you for the feedback.

"At this point, I am thinking it might be best to make a new folder in the combinat folder called tableau with your new code (one file for the ABC and your other two files). Then it will become the eventual home for all tableau files (but moving those should be done on separate tickets)."

Could you please expand on this? I understand your suggestion to use an ABC instead of a category. I am not clear on how this should be done within the trac system. I understand that I could open a new ticket, copy the files by hand, and at some point delete the existing ticket. However I suspect you may mean a different approach.

Is the first step to create a new ticket with the new folder called tableau containing just one file with the ABC?

comment:21 Changed 4 years ago by gh-darijgr

You can keep making commits and pushing to the same branch (and set the ticket back to needs_review once you're done fixing the issues you're aware of).

comment:22 Changed 4 years ago by git

  • Commit changed from 0ae66fe9d4bdf4c90b3ccc1211baa692f66a3311 to cdef4c8244107e9fee3c3067664f0ff86df8bb9e

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

cdef4c8Corrections to doctests with ellipsis

comment:23 Changed 4 years ago by git

  • Commit changed from cdef4c8244107e9fee3c3067664f0ff86df8bb9e to d5622463ac055afec1f2968209bc8b4373d5c8ff

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

d562246Files moved to new directory

comment:24 Changed 4 years ago by git

  • Commit changed from d5622463ac055afec1f2968209bc8b4373d5c8ff to 5aa14e11073115603e701cd132ff9a744d123836

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

8d0191aEdited and added files all.py
18aaf93File __init__.py added
5aa14e1Moved directory from tableau to pathtableau

comment:25 Changed 4 years ago by git

  • Commit changed from 5aa14e11073115603e701cd132ff9a744d123836 to f20a5d192e219d7a3420431da6002a95fd303a27

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

f20a5d1Parent classes added

comment:26 Changed 4 years ago by git

  • Commit changed from f20a5d192e219d7a3420431da6002a95fd303a27 to 4adad87c4e9a9bd635848798050a366baff2df2d

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

4adad87All tests passed!

comment:27 Changed 4 years ago by git

  • Commit changed from 4adad87c4e9a9bd635848798050a366baff2df2d to 9c0cd75f314c958b0df9841d43358f6a01b283c7

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

9c0cd75PathTableau_partitions added and more doctests

comment:28 Changed 4 years ago by Bruce

  • Status changed from needs_work to needs_info

I have made the changes suggested by Travis, including removing the Category and using an ABC instead. I am working on missing doctests. I would appreciate further feedback.

comment:29 Changed 4 years ago by deinst

  • Cc deinst added

comment:30 Changed 4 years ago by Bruce

  • Description modified (diff)
  • Status changed from needs_info to needs_review

comment:31 Changed 4 years ago by git

  • Commit changed from 9c0cd75f314c958b0df9841d43358f6a01b283c7 to 8715ae9bc8344eac675dcb6dc950b6bb0c5df021

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

f4d341fmultiply doctest added
7cb9d1fMore doctests
8b21628rule changed to avoid use of vector
c751cc0Moved tests. Use _test_ and other minor changes on tests.
bb2caa0Moved PathTableaux class back
8715ae9All doctests pass

comment:32 Changed 4 years ago by git

  • Commit changed from 8715ae9bc8344eac675dcb6dc950b6bb0c5df021 to 800560cff5fcb0136736c25a3cb474c013da47c8

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

800560cModified all.py

comment:33 Changed 4 years ago by kdilks

  • Cc kdilks added

comment:34 Changed 4 years ago by git

  • Commit changed from 800560cff5fcb0136736c25a3cb474c013da47c8 to 6229e0d6554077c74a5b8c47723fe56bce626a2f

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

6229e0dSemistandard removed

comment:35 Changed 4 years ago by bsalisbury1

  • Branch changed from u/Bruce/pathtableaux to u/bsalisbury1/pathtableaux

comment:36 Changed 4 years ago by git

  • Commit changed from 6229e0d6554077c74a5b8c47723fe56bce626a2f to efdd91d74bd3b437a4e0114dfefba8fb695a0c2a

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

efdd91dDocumentation updates

comment:37 Changed 4 years ago by git

  • Commit changed from efdd91d74bd3b437a4e0114dfefba8fb695a0c2a to a3a6472ce2f82f2017e087ea9d55d5e02f317f13

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

a3a6472Doctests added

comment:38 Changed 4 years ago by bsalisbury1

  • Reviewers set to Ben Salisbury
  • Status changed from needs_review to positive_review

comment:39 Changed 4 years ago by kdilks

Is from sage.combinat.pathtableau.all import * right? I don't think the .all is supposed to be there. Tried to run patchbot and immediately crashed because of that.

comment:40 Changed 4 years ago by git

  • Commit changed from a3a6472ce2f82f2017e087ea9d55d5e02f317f13 to f4b5642cbbfc2eb13c1d10299560c5af84437b18
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

f4b5642updated all.py

comment:41 Changed 4 years ago by git

  • Commit changed from f4b5642cbbfc2eb13c1d10299560c5af84437b18 to 3d2f309cbe680bf1004fa83f363e06aca68be00b

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

3d2f309git added all.py and __init__.py

comment:42 Changed 4 years ago by bsalisbury1

  • Status changed from needs_review to positive_review

comment:43 Changed 4 years ago by kdilks

For python 3 compatability, the two print commands in commutor() should be changed from 'print stuff' to 'print(stuff)' .

comment:44 Changed 4 years ago by Bruce

The method _rule_ should have @staticmethod (before @abstractmethod)

comment:45 Changed 4 years ago by Bruce

There should be references.

comment:46 follow-up: Changed 4 years ago by tscrim

  • Status changed from positive_review to needs_work

I also think we should rename _rule_ -> _rule as methods with a leading and trailing underscores (_) are generally considered as special Sage functions.

In addition to the Python3 compatibility mentioned in commment:43, the test for dual_equivalence_graph seems quite brittle. It would be better to explicitly construct the graph and compare it by isomorphism.

I don't see why you have a PathTableaux class. It look like it is a very malformed parent.

comment:47 Changed 4 years ago by embray

I would also like to test this ticket on top of my python 3 branch, but that is up to me to do. Not that python 3 is technically supported yet, so if there are any significant issues in other parts of Sage that affect this code on python 3 then I won't worry about it. But it would be nice to keep significant new code forward-compatible where possible.

comment:48 follow-up: Changed 4 years ago by Bruce

  • Branch changed from u/bsalisbury1/pathtableaux to u/Bruce/pathtableaux

comment:49 follow-up: Changed 4 years ago by git

  • Commit changed from 3d2f309cbe680bf1004fa83f363e06aca68be00b to 340117e2904c628dd1f75c8366b6ec7fa7919468

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

baae1dfReference added
a716ba8Reference changed
e36c89bFixed reference
52b8488Ref
f03e476Ref
295aeebRef
269ded8Ref
340117eRef

comment:50 in reply to: ↑ 46 Changed 4 years ago by Bruce

Replying to tscrim:

the test for dual_equivalence_graph seems quite brittle. It would be better to explicitly construct the graph and compare it by isomorphism.

I understand this to mean my code could construct the wrong graph but this test would not notice if it happened to have the same adjacency matrix. Really? or did you mean something else?

I don't see why you have a PathTableaux class. It look like it is a very malformed parent.

Could you please expand on this? I take malformed to be pejorative. I will want these to be (finite) enumerated sets but I haven't thought this through yet.

comment:51 in reply to: ↑ 48 Changed 4 years ago by Bruce

Replying to Bruce: This takes care of comments 44, 45 and the first sentence in 47.

comment:52 in reply to: ↑ 49 Changed 4 years ago by Bruce

Replying to git:

This takes care of comment 46.

comment:53 Changed 4 years ago by Bruce

It seems I could use RecursivelyEnumeratedSet() to give a generic enumeration. Just because I can do something doesn't mean I should.

Does anyone think this would be reasonable? or unreasonable?

comment:54 follow-up: Changed 4 years ago by mantepse

You could have a look at #25173, where I have done this for staircase tableaux. Also, I am a bit surprised that you do not reference our joint paper, I must say.

Last edited 4 years ago by mantepse (previous) (diff)

comment:55 in reply to: ↑ 54 Changed 4 years ago by Bruce

Replying to mantepse:

You could have a look at #25173, where I have done this for staircase tableaux. Also, I am a bit surprised that you do not reference our joint paper, I must say.

Is there any code for #25173 (apart from sl3webs.sage? or is it a "wish list"?

I will refer to our joint paper in the relevant implementations. At the moment I am concentrating on getting a positive review for the ABC and the toy implementation.

comment:56 Changed 4 years ago by mantepse

sl3webs.sage contains (possibly) relevant code. Just look for "RecursivelyEnumeratedSet?"!

comment:57 Changed 4 years ago by git

  • Commit changed from 340117e2904c628dd1f75c8366b6ec7fa7919468 to 5dcc3364869ff867afb6def5616fcc43432d1002

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

c3cde2eUsed clone
5dcc336Another clone

comment:58 Changed 4 years ago by git

  • Commit changed from 5dcc3364869ff867afb6def5616fcc43432d1002 to 124c505519c3e6b734b3c2ea6d62036b953e0244

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

d346b00Element added
88862d6Element imported
124c505Sets

comment:59 Changed 4 years ago by git

  • Commit changed from 124c505519c3e6b734b3c2ea6d62036b953e0244 to 212d2adf2c13487bdec028424e3941f5a0139b46

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

954048eElement removed
57c76e2Directory and filename changed
8df41acEdit to all.py
212d2adCorrected names

comment:60 Changed 4 years ago by Bruce

  • Status changed from needs_work to needs_review

comment:61 Changed 4 years ago by Bruce

  • Status changed from needs_review to needs_work

comment:62 Changed 4 years ago by Bruce

I am working on implementing RecursiveEnumeratedSets? and introducing Parents whose category is EnumeratedSets?. I am struggling as I don't have an example to follow; DyckWords? is the nearest I have found but this uses a deprecated backtracker.

comment:63 Changed 4 years ago by git

  • Commit changed from 212d2adf2c13487bdec028424e3941f5a0139b46 to 236ae8bc003a23b1b404ec4059b029f240e8ee3d

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

e2c3b14Added __getattr__ and several Parents
830636dAdded __getatrr__ and other minor changes
d60ce49More work
236ae8bAdded construction from skew tableau

comment:64 Changed 4 years ago by git

  • Commit changed from 236ae8bc003a23b1b404ec4059b029f240e8ee3d to ee430d5342bb324616c71d9c43e08ad7b03b752b

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

11c5eb6File names changed in __init__.py
0818203Made _rule a helper function.
ee430d5Made _rule a helper function

comment:65 Changed 4 years ago by git

  • Commit changed from ee430d5342bb324616c71d9c43e08ad7b03b752b to bcd1acd7a8935fa7be061aa616f964b334817d84

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

bcd1acdTidy up

comment:66 Changed 4 years ago by gh-BruceWestbury

  • Status changed from needs_work to needs_review

I have now made all modifications following conversations at ICERM and subsequent comments. The only one I am not sure about is whether I have correctly dealt with Travis' comment on "malformed parents".

I would be grateful for a review.

comment:67 Changed 4 years ago by tscrim

  • Milestone changed from sage-8.3 to sage-8.4
  • Reviewers changed from Ben Salisbury to Ben Salisbury, Travis Scrimshaw
  • Status changed from needs_review to needs_work

Now you have a proper parent and element. So that part is a good improvement over trying to do things with categories.

A naming convention, you should call the folder path_tableaux. For filenames, etc. that are all lowercase, words are separated by underscores _.

I believe you are better using ClonableArray instead of ClonableList because your object should be a fixed length because it is effectively immutable.

All methods need a doctest, including __classcall_private__ and the abstract _local_rule.

Instead of display, the somewhat standard argument name is verbose.

I think your __getattr__ hack is dangerous and violates the principle of least surprise. It is dangerous because suppose you have two PathTableau that convert to each other but neither implements a given method. My understanding is that would lead to an infinite loop. It is also slow because it has to do and then check for each such conversion. Nor does not dispatch to base classes, which might interfere with some of the hacks of the category framework (I would have to check this one specifically, but this might not be an issue because this is not a Cython class).

This is also extremely brittle as it depends heavily on implementations in other unrelated classes (violating code locality) and the result depends strongly on the order of _conversions. Which brings me to the violation the principle of least surprise. Suddenly you get a DyckPath instead of a PathTableau when asking for a method that might be natural or nonsense:

sage: t = CatalanTableau([0,1,2,3,2,1,0])
sage: t.to_DyckWord()
[1, 1, 1, 0, 0, 0]
sage: _.reverse()
[1, 1, 1, 0, 0, 0]
sage: t.reverse()
[1, 1, 1, 0, 0, 0]

These are PathTableau, so either you implement methods directly there or you explicitly convert.

The rest of it mostly looks good other than perhaps a few small formatting tweaks. I can address those once the above is taken care of.

comment:68 Changed 4 years ago by git

  • Commit changed from bcd1acd7a8935fa7be061aa616f964b334817d84 to ec639273360dfd10575c3980230cc7fa82bc2014

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

e25e2aaMerge branch 'develop' into t/25434/pathtableaux
6518f21Dealt with Travis' objection to my __getattr__ hack and most of his other comments.
ec63927Made minor changes to catalan.py

comment:69 Changed 4 years ago by git

  • Commit changed from ec639273360dfd10575c3980230cc7fa82bc2014 to 653f8a5630af1fe8241da52c9690418848941d5b

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

12b27c4Changed directory name
653f8a5Changed import to new file names

comment:70 Changed 4 years ago by Bruce

  • Status changed from needs_work to needs_review

I believe I have taken care of Travis' comments.

comment:71 Changed 4 years ago by git

  • Commit changed from 653f8a5630af1fe8241da52c9690418848941d5b to 03152b8c8fd9e293083b3a344786f09ed0016fa6

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

03152b8Moved ClonableArray from ABC class to implementation

comment:72 Changed 3 years ago by kdilks

Realized that I should probably focus on this one before (or at least concurrently with) oscillating tableau. Travis has hopefully covered all the bases in terms of the programming technicalities (and should probably confirm before this gets a final positive review), I'll focus more on doc/usability.

Some of my previous comments from oscillating tableau actually apply to this ticket (I didn't realize this ticket was being pulled into oscillating tableau ticket as a dependency).

  • Adding the underscores to the folder/file names requires a few other lines to be changed. In src/doc/en/reference/combinat/module_list.rst, the line sage/combinat/pathtableau/catalan needs to be updated to sage/combinat/path_tableau/catalan. Also, sage/combinat/pathtableau/pathtableaux needs to be updated to sage/combinat/path_tableau/path_tableaux. Finally, in CatalanTableaux?, the link :class:`sage.combinat.pathtableau.pathtableaux` needs to be updated to :class:`sage.combinat.path_tableau.path_tableaux`.
  • descents method needs some sort of definition, because in particular, whatever definition is used here doesn't agree with my first idea of what it should be (i appearing in a row below i+1 in the associated two row tableau.
  • to_perfect_matching should first check to see if the tableau is skew, and give an appropriate error message (rather than having the call to DyckWord? fail). Furthermore, if you input something that doesn't end at 0 (corresponding to a two-row tableau with uneven rows), the code still returns a perfect matching, and I'm not sure this is desired behavior.
  • I would modify to_tableau some, because it should be possible to have this return a StandardTableau? in the case where you start with 0 and aren't actually skew. I would have the default behavior check is_skew, and then use the appropriate constructor. I suppose there could be situations where you always want your output to use the SkewTableau? constructor, so there could be some optional parameter like skew=False that you could set to be True if you always wanted to use the SkewTableau? constructor.
  • PathTableaux? needs some kind of general definition of what path tableaux actually are. It should appear both with the current description of what it's trying to abstract (which should appear at the top of the documentation page for the module), and also for the PathTableau? class itself (so it will show up when viewing PathTableau?? ).
  • Right now, there's just a method to apply the action of a single cactus group generator. It may be useful to have a method that would take a list/sequence of generators and apply them successively.
  • The method commutor should give some kind of general definition of what the commutor actually is.

comment:73 Changed 3 years ago by kdilks

  • Branch changed from u/Bruce/pathtableaux to u/kdilks/pathtableaux

comment:74 Changed 3 years ago by git

  • Commit changed from 03152b8c8fd9e293083b3a344786f09ed0016fa6 to 415e70444f2535292ff8dae47a2352be3b34ce49

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

415e704I can't spell in French

comment:75 Changed 3 years ago by git

  • Commit changed from 415e70444f2535292ff8dae47a2352be3b34ce49 to cd325b4f3f47d295ea9c5dc03a9e5150994ccdf2

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

9156fb4Merge branch 'u/kdilks/pathtableaux' of git://trac.sagemath.org/sage into pathtab25434
4f51907Moved doc from classcall, added check in to_perfect_matching for skewness
cd325b4modified to_tableau method

comment:76 Changed 3 years ago by Bruce

  • Branch changed from u/kdilks/pathtableaux to u/Bruce/pathtableaux

comment:77 Changed 3 years ago by git

  • Commit changed from cd325b4f3f47d295ea9c5dc03a9e5150994ccdf2 to 1b7e13494daaa8e90a684e74dab69f99837c8866

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

1b7e134Edited doc strings.

comment:78 Changed 3 years ago by tscrim

  • Keywords fpsac2019 added
  • Milestone changed from sage-8.4 to sage-8.9
  • Status changed from needs_review to needs_work

EXAMPLE:: -> EXAMPLES:: which should also have a blankline after it.

Many methods do not have doctests.

Remove the CylndricalDiagram.__str__ and change the __repr__ -> _repr_. You should also implement an _ascii_art_ (and possibly a _unicode_art_) and have pp call that.

For the _test_involution, I would move the all out and do an explicit for loop that tests each pair (better control of what fails). Also this way you can also use tester._max_runs in case size() is really big.

PathTableau should inherit from ClonableArray. IMO your divider comments in that class should be indented to make it easier to read too.

I don't see the point of the _rule function in _local_rule. Just put that in the for loop.

What follows after an INPUT: block should *not* be indented.

The docstring for catalan.py should be first, not the copyright

comment:79 Changed 3 years ago by git

  • Commit changed from 1b7e13494daaa8e90a684e74dab69f99837c8866 to 544eb660c2bd50076c4622b5e2c7d4c7c7f1e0ab

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

716b7ceAdded __init__ in CatalanTableau
544eb66Made corrections requested by Travis.

comment:80 Changed 3 years ago by tscrim

Is this suppose to be back to needs review?

comment:81 Changed 3 years ago by chapoton

  • never use EXAMPLES:: twice in the doc for one method or function
  • There is a missing empty line after one EXAMPLES::
  • please do not add empty methods
Last edited 3 years ago by chapoton (previous) (diff)

comment:82 Changed 3 years ago by embray

  • Milestone changed from sage-8.9 to sage-9.1

Ticket retargeted after milestone closed

comment:83 Changed 2 years ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-9.2

Batch modifying tickets that will likely not be ready for 9.1, based on a review of the ticket title, branch/review status, and last modification date.

comment:84 Changed 2 years ago by git

  • Commit changed from 544eb660c2bd50076c4622b5e2c7d4c7c7f1e0ab to 7884fc5ec26c2b7b2bcf530bca55f60d38cd6476

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

03ce5ddNo idea
ce528c3Merge branch 'develop' into t/25434/pathtableaux
2473d5bMinor edits
f3dc239Worked on doc tests
3d82797All tests passed!
009466dAdded doc tests
7884fc5More tests

comment:85 Changed 2 years ago by Bruce

I have improved the coverage of documentation and testing in response to comments by Scrimshaw and Chapoton. There were some places where I was not confident I was getting it right.

comment:86 Changed 2 years ago by Bruce

  • Status changed from needs_work to needs_review

comment:87 Changed 2 years ago by chapoton

Looking at the patchbot report, one sees:

In the first lines of method docs, replace "Returns" by "Return":

++        Returns a string representation of ``self``
++        Returns a `\LaTeX` representation of ``self``
++        Returns an ascii art representation of ``self``
++        Returns a unicode art representation of ``self``

There is one function missing documentation:

+combinat/path_tableaux/path_tableau.py: 95.8% (23 of 24)

Moreover, by a quick look at the ticket:

Also, always have an empty line after the first line of the doc of any function or method.

comment:88 Changed 2 years ago by git

  • Commit changed from 7884fc5ec26c2b7b2bcf530bca55f60d38cd6476 to c0c9e5c11a61276e5eb9519c76f2e6a19d668113

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

c0c9e5cTidied documentation

comment:89 Changed 2 years ago by Bruce

I have fixed the above.

The method without a doctest is _element_constructor_ on line 422

I am not sure when this gets called. I assume it has been called several times implicitly in the existing doctests and I don't know what an explicit doctest would do.

comment:90 Changed 2 years ago by git

  • Commit changed from c0c9e5c11a61276e5eb9519c76f2e6a19d668113 to f5887d6bf505aba836b28351574229df6ee3c864

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

f5887d6Removed trailing spaces

comment:91 Changed 2 years ago by chapoton

For the element_constructor doctest, you can add one or several creations of elements, tagged with # indirect doctest

comment:92 Changed 2 years ago by chapoton

From comment:78

What follows after an INPUT: block should *not* be indented.

has not been taken care of yet.

comment:93 Changed 2 years ago by chapoton

Each raise statement must be triggered in one doctest. For example

raise ValueError("the perfect matching must be non crossing")

comment:94 Changed 2 years ago by chapoton

Still some empty lines missing, for instance

+        TESTS::
+            sage: CatalanTableau([0,1,0,-1,0])

comment:95 Changed 2 years ago by git

  • Commit changed from f5887d6bf505aba836b28351574229df6ee3c864 to ddf1a4b11ec182744633903da256a639b29af1be

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

ddf1a4bCompleted doctests

comment:96 Changed 2 years ago by git

  • Commit changed from ddf1a4b11ec182744633903da256a639b29af1be to 7866ab4812259aba6fb241cde7b666302d461bec

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

51d3b73Added tests of ValueError
7866ab4More tests

comment:97 Changed 2 years ago by chapoton

Travis, any opinion ? I am not sure that we want these things in the global namespace.

comment:98 Changed 2 years ago by tscrim

I am inclined to agree with you. It probably would be best as a catalog of the form path_tableaux.Name and have that be in the global namespace.

There are also a few places (e.g., in to_word()) that need the docstring to being r""" (although I would argue you should essentially always do this to be safe).

There are also a few other places where the docstrings are not formatted for Sage (cactus has $'s, PathTableaux really has a one-line docstring (instead of having the """ on their own lines), one-line docstrings should end in a period/full-stop, etc.) However these are very minor things.

comment:99 Changed 2 years ago by git

  • Commit changed from 7866ab4812259aba6fb241cde7b666302d461bec to 387c0bd94ed25057df5003e6ec7ae38018191524

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

387c0bdMinor changes in doc strings

comment:100 Changed 2 years ago by git

  • Commit changed from 387c0bd94ed25057df5003e6ec7ae38018191524 to 767405c654f189c298b6f5da035d68db4de72219

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

4747bd6Added catalog.py and changed name
8567a47Changed _local_rule to local_rule
767405cTried to write catalog.py

comment:101 Changed 2 years ago by git

  • Commit changed from 767405c654f189c298b6f5da035d68db4de72219 to 62e7e6312a7d527aa811db7f9686201bab5ad716

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

0942d3bEdited combinat/all.py all.py etc. to import from catalog
62e7e63Minor edits

comment:102 Changed 2 years ago by tscrim

  • Branch changed from u/Bruce/pathtableaux to public/combinat/path_tableaux-25434
  • Commit changed from 62e7e6312a7d527aa811db7f9686201bab5ad716 to bc8119f90fd742a98010f8f7e7604df2bcae2d96

I did some revisions to get everything up to Sage's standards and make the catalog be lazily imported (and the only entry point). In particular, I removed the PathTableau[x] from the catalog since they are ABCs.


New commits:

214c067Merge branch 'u/Bruce/pathtableaux' of git://trac.sagemath.org/sage into public/combinat/path_tableaux-25434
bc8119fDoing some cleanup to pass doctests, better test coverage, and lazy importing the catalog.

comment:103 Changed 2 years ago by Bruce

Thanks Travis.

comment:104 Changed 2 years ago by git

  • Commit changed from bc8119f90fd742a98010f8f7e7604df2bcae2d96 to ad8ca4de3dcb1b9b217c5363443dbfe113d84a1b

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

ad8ca4dFixing the docbuild by having the correct files listed.

comment:105 Changed 2 years ago by tscrim

Green patchbot.

comment:106 Changed 2 years ago by tscrim

So are my changes good? Is this a positive review?

comment:107 Changed 2 years ago by tscrim

  • Status changed from needs_review to needs_work

One more thing that needs to be done: the DyckPaths needs an _an_element_ method. Then you can change this test, which is wrong:

-            sage: t = path_tableaux.DyckPath([0,1,2,1,0])
+            sage: t = path_tableaux.DyckPaths()
             sage: TestSuite(t).run()

comment:108 Changed 2 years ago by git

  • Commit changed from ad8ca4de3dcb1b9b217c5363443dbfe113d84a1b to 9d3a838478a8c7f218bf333c2a58f598c2fe7f8b

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

b248c6aChanged to one line if
79192f9Merge branch 'public/combinat/path_tableaux-25434' of git://trac.sagemath.org/sage into t/25434/pathtableaux
29732ebAdded _an_element_
9d3a838Merge branch 'public/combinat/path_tableaux-25434' of git://trac.sagemath.org/sage into t/25434/pathtableaux

comment:109 Changed 2 years ago by Bruce

  • Status changed from needs_work to needs_review

comment:110 Changed 2 years ago by tscrim

  • Status changed from needs_review to positive_review

Thank you.

comment:111 Changed 2 years ago by vbraun

  • Branch changed from public/combinat/path_tableaux-25434 to 9d3a838478a8c7f218bf333c2a58f598c2fe7f8b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.