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: |
Description (last modified by )
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
comment:2 Changed 4 years ago by
- 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
- Branch set to u/Bruce/pathtableaux
comment:4 Changed 4 years ago by
- Commit set to b4a14f9782c9584fa79344e4902eb147810ebac5
- Description modified (diff)
- Owner changed from (none) to Bruce
comment:5 Changed 4 years ago by
- Cc mantepse aschilling added
comment:6 Changed 4 years ago by
- 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: ↓ 8 Changed 4 years ago by
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: ↓ 13 Changed 4 years ago by
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
- Branch set to u/Bruce/path_tableaux
comment:10 Changed 4 years ago by
- Branch changed from u/Bruce/path_tableaux to u/Bruce/pathtableaux
- Commit set to 3090a75ec766aa05f83ee6046cabed79b7abe369
comment:11 Changed 4 years ago by
- Commit changed from 3090a75ec766aa05f83ee6046cabed79b7abe369 to 8944c6e6c854148d2f884346663c305a7c6990f5
comment:12 Changed 4 years ago by
- Commit changed from 8944c6e6c854148d2f884346663c305a7c6990f5 to 7d6fef08c58ddce1e3dea7b52ff4081ead34c245
Branch pushed to git repo; I updated commit sha1. New commits:
7d6fef0 | Imports into semistandard.py fixed
|
comment:13 in reply to: ↑ 8 Changed 4 years ago by
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
- Commit changed from 7d6fef08c58ddce1e3dea7b52ff4081ead34c245 to 391ffd0c447336a2459b2a87265bb246372bd71d
comment:15 Changed 4 years ago by
- Commit changed from 391ffd0c447336a2459b2a87265bb246372bd71d to 16c04c43e726fe41005c3346e672503a9702277d
comment:16 Changed 4 years ago by
- Commit changed from 16c04c43e726fe41005c3346e672503a9702277d to e5cb8617c06746325523d815a6c28728a225e4a1
Branch pushed to git repo; I updated commit sha1. New commits:
e5cb861 | Rectify added
|
comment:17 Changed 4 years ago by
- 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
- 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 ofdraw
. - Your
drawL
anddrawC
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
indrawL
as an internal helper function (indrawC
, it makes sense because of the twofor
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
- Commit changed from e5cb8617c06746325523d815a6c28728a225e4a1 to 0ae66fe9d4bdf4c90b3ccc1211baa692f66a3311
Branch pushed to git repo; I updated commit sha1. New commits:
0ae66fe | Corrected formatting of doctests
|
comment:20 Changed 4 years ago by
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
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
- Commit changed from 0ae66fe9d4bdf4c90b3ccc1211baa692f66a3311 to cdef4c8244107e9fee3c3067664f0ff86df8bb9e
Branch pushed to git repo; I updated commit sha1. New commits:
cdef4c8 | Corrections to doctests with ellipsis
|
comment:23 Changed 4 years ago by
- Commit changed from cdef4c8244107e9fee3c3067664f0ff86df8bb9e to d5622463ac055afec1f2968209bc8b4373d5c8ff
Branch pushed to git repo; I updated commit sha1. New commits:
d562246 | Files moved to new directory
|
comment:24 Changed 4 years ago by
- Commit changed from d5622463ac055afec1f2968209bc8b4373d5c8ff to 5aa14e11073115603e701cd132ff9a744d123836
comment:25 Changed 4 years ago by
- Commit changed from 5aa14e11073115603e701cd132ff9a744d123836 to f20a5d192e219d7a3420431da6002a95fd303a27
Branch pushed to git repo; I updated commit sha1. New commits:
f20a5d1 | Parent classes added
|
comment:26 Changed 4 years ago by
- Commit changed from f20a5d192e219d7a3420431da6002a95fd303a27 to 4adad87c4e9a9bd635848798050a366baff2df2d
Branch pushed to git repo; I updated commit sha1. New commits:
4adad87 | All tests passed!
|
comment:27 Changed 4 years ago by
- Commit changed from 4adad87c4e9a9bd635848798050a366baff2df2d to 9c0cd75f314c958b0df9841d43358f6a01b283c7
Branch pushed to git repo; I updated commit sha1. New commits:
9c0cd75 | PathTableau_partitions added and more doctests
|
comment:28 Changed 4 years ago by
- 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
- Cc deinst added
comment:30 Changed 4 years ago by
- Description modified (diff)
- Status changed from needs_info to needs_review
comment:31 Changed 4 years ago by
- Commit changed from 9c0cd75f314c958b0df9841d43358f6a01b283c7 to 8715ae9bc8344eac675dcb6dc950b6bb0c5df021
comment:32 Changed 4 years ago by
- Commit changed from 8715ae9bc8344eac675dcb6dc950b6bb0c5df021 to 800560cff5fcb0136736c25a3cb474c013da47c8
Branch pushed to git repo; I updated commit sha1. New commits:
800560c | Modified all.py
|
comment:33 Changed 4 years ago by
- Cc kdilks added
comment:34 Changed 4 years ago by
- Commit changed from 800560cff5fcb0136736c25a3cb474c013da47c8 to 6229e0d6554077c74a5b8c47723fe56bce626a2f
Branch pushed to git repo; I updated commit sha1. New commits:
6229e0d | Semistandard removed
|
comment:35 Changed 4 years ago by
- Branch changed from u/Bruce/pathtableaux to u/bsalisbury1/pathtableaux
comment:36 Changed 4 years ago by
- Commit changed from 6229e0d6554077c74a5b8c47723fe56bce626a2f to efdd91d74bd3b437a4e0114dfefba8fb695a0c2a
Branch pushed to git repo; I updated commit sha1. New commits:
efdd91d | Documentation updates
|
comment:37 Changed 4 years ago by
- Commit changed from efdd91d74bd3b437a4e0114dfefba8fb695a0c2a to a3a6472ce2f82f2017e087ea9d55d5e02f317f13
Branch pushed to git repo; I updated commit sha1. New commits:
a3a6472 | Doctests added
|
comment:38 Changed 4 years ago by
- Reviewers set to Ben Salisbury
- Status changed from needs_review to positive_review
comment:39 Changed 4 years ago by
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
- 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:
f4b5642 | updated all.py
|
comment:41 Changed 4 years ago by
- Commit changed from f4b5642cbbfc2eb13c1d10299560c5af84437b18 to 3d2f309cbe680bf1004fa83f363e06aca68be00b
Branch pushed to git repo; I updated commit sha1. New commits:
3d2f309 | git added all.py and __init__.py
|
comment:42 Changed 4 years ago by
- Status changed from needs_review to positive_review
comment:43 Changed 4 years ago by
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
The method _rule_ should have @staticmethod (before @abstractmethod)
comment:45 Changed 4 years ago by
There should be references.
comment:46 follow-up: ↓ 50 Changed 4 years ago by
- 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
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: ↓ 51 Changed 4 years ago by
- Branch changed from u/bsalisbury1/pathtableaux to u/Bruce/pathtableaux
comment:49 follow-up: ↓ 52 Changed 4 years ago by
- Commit changed from 3d2f309cbe680bf1004fa83f363e06aca68be00b to 340117e2904c628dd1f75c8366b6ec7fa7919468
comment:50 in reply to: ↑ 46 Changed 4 years ago by
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
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
Replying to git:
This takes care of comment 46.
comment:53 Changed 4 years ago by
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: ↓ 55 Changed 4 years ago by
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.
comment:55 in reply to: ↑ 54 Changed 4 years ago by
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
sl3webs.sage
contains (possibly) relevant code. Just look for "RecursivelyEnumeratedSet?"!
comment:57 Changed 4 years ago by
- Commit changed from 340117e2904c628dd1f75c8366b6ec7fa7919468 to 5dcc3364869ff867afb6def5616fcc43432d1002
comment:58 Changed 4 years ago by
- Commit changed from 5dcc3364869ff867afb6def5616fcc43432d1002 to 124c505519c3e6b734b3c2ea6d62036b953e0244
comment:59 Changed 4 years ago by
- Commit changed from 124c505519c3e6b734b3c2ea6d62036b953e0244 to 212d2adf2c13487bdec028424e3941f5a0139b46
comment:60 Changed 4 years ago by
- Status changed from needs_work to needs_review
comment:61 Changed 4 years ago by
- Status changed from needs_review to needs_work
comment:62 Changed 4 years ago by
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
- Commit changed from 212d2adf2c13487bdec028424e3941f5a0139b46 to 236ae8bc003a23b1b404ec4059b029f240e8ee3d
comment:64 Changed 4 years ago by
- Commit changed from 236ae8bc003a23b1b404ec4059b029f240e8ee3d to ee430d5342bb324616c71d9c43e08ad7b03b752b
comment:65 Changed 4 years ago by
- Commit changed from ee430d5342bb324616c71d9c43e08ad7b03b752b to bcd1acd7a8935fa7be061aa616f964b334817d84
Branch pushed to git repo; I updated commit sha1. New commits:
bcd1acd | Tidy up
|
comment:66 Changed 4 years ago by
- 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
- 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
- Commit changed from bcd1acd7a8935fa7be061aa616f964b334817d84 to ec639273360dfd10575c3980230cc7fa82bc2014
comment:69 Changed 4 years ago by
- Commit changed from ec639273360dfd10575c3980230cc7fa82bc2014 to 653f8a5630af1fe8241da52c9690418848941d5b
comment:70 Changed 4 years ago by
- Status changed from needs_work to needs_review
I believe I have taken care of Travis' comments.
comment:71 Changed 4 years ago by
- Commit changed from 653f8a5630af1fe8241da52c9690418848941d5b to 03152b8c8fd9e293083b3a344786f09ed0016fa6
Branch pushed to git repo; I updated commit sha1. New commits:
03152b8 | Moved ClonableArray from ABC class to implementation
|
comment:72 Changed 3 years ago by
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 linesage/combinat/pathtableau/catalan
needs to be updated tosage/combinat/path_tableau/catalan
. Also,sage/combinat/pathtableau/pathtableaux
needs to be updated tosage/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`
.
- As with OscillatingTableaux?, the docstrings for CatalanTableaux? need to be moved from _classcall_private_ up to the class itself.
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 checkis_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
- Branch changed from u/Bruce/pathtableaux to u/kdilks/pathtableaux
comment:74 Changed 3 years ago by
- Commit changed from 03152b8c8fd9e293083b3a344786f09ed0016fa6 to 415e70444f2535292ff8dae47a2352be3b34ce49
Branch pushed to git repo; I updated commit sha1. New commits:
415e704 | I can't spell in French
|
comment:75 Changed 3 years ago by
- Commit changed from 415e70444f2535292ff8dae47a2352be3b34ce49 to cd325b4f3f47d295ea9c5dc03a9e5150994ccdf2
comment:76 Changed 3 years ago by
- Branch changed from u/kdilks/pathtableaux to u/Bruce/pathtableaux
comment:77 Changed 3 years ago by
- Commit changed from cd325b4f3f47d295ea9c5dc03a9e5150994ccdf2 to 1b7e13494daaa8e90a684e74dab69f99837c8866
Branch pushed to git repo; I updated commit sha1. New commits:
1b7e134 | Edited doc strings.
|
comment:78 Changed 3 years ago by
- 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
- Commit changed from 1b7e13494daaa8e90a684e74dab69f99837c8866 to 544eb660c2bd50076c4622b5e2c7d4c7c7f1e0ab
comment:80 Changed 3 years ago by
Is this suppose to be back to needs review?
comment:81 Changed 3 years ago by
- 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
comment:82 Changed 3 years ago by
- Milestone changed from sage-8.9 to sage-9.1
Ticket retargeted after milestone closed
comment:83 Changed 2 years ago by
- 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
- Commit changed from 544eb660c2bd50076c4622b5e2c7d4c7c7f1e0ab to 7884fc5ec26c2b7b2bcf530bca55f60d38cd6476
comment:85 Changed 2 years ago by
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
- Status changed from needs_work to needs_review
comment:87 Changed 2 years ago by
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
- Commit changed from 7884fc5ec26c2b7b2bcf530bca55f60d38cd6476 to c0c9e5c11a61276e5eb9519c76f2e6a19d668113
Branch pushed to git repo; I updated commit sha1. New commits:
c0c9e5c | Tidied documentation
|
comment:89 Changed 2 years ago by
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
- Commit changed from c0c9e5c11a61276e5eb9519c76f2e6a19d668113 to f5887d6bf505aba836b28351574229df6ee3c864
Branch pushed to git repo; I updated commit sha1. New commits:
f5887d6 | Removed trailing spaces
|
comment:91 Changed 2 years ago by
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
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
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
Still some empty lines missing, for instance
+ TESTS:: + sage: CatalanTableau([0,1,0,-1,0])
comment:95 Changed 2 years ago by
- Commit changed from f5887d6bf505aba836b28351574229df6ee3c864 to ddf1a4b11ec182744633903da256a639b29af1be
Branch pushed to git repo; I updated commit sha1. New commits:
ddf1a4b | Completed doctests
|
comment:96 Changed 2 years ago by
- Commit changed from ddf1a4b11ec182744633903da256a639b29af1be to 7866ab4812259aba6fb241cde7b666302d461bec
comment:97 Changed 2 years ago by
Travis, any opinion ? I am not sure that we want these things in the global namespace.
comment:98 Changed 2 years ago by
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
- Commit changed from 7866ab4812259aba6fb241cde7b666302d461bec to 387c0bd94ed25057df5003e6ec7ae38018191524
Branch pushed to git repo; I updated commit sha1. New commits:
387c0bd | Minor changes in doc strings
|
comment:100 Changed 2 years ago by
- Commit changed from 387c0bd94ed25057df5003e6ec7ae38018191524 to 767405c654f189c298b6f5da035d68db4de72219
comment:101 Changed 2 years ago by
- Commit changed from 767405c654f189c298b6f5da035d68db4de72219 to 62e7e6312a7d527aa811db7f9686201bab5ad716
comment:102 Changed 2 years ago by
- 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:
214c067 | Merge branch 'u/Bruce/pathtableaux' of git://trac.sagemath.org/sage into public/combinat/path_tableaux-25434
|
bc8119f | Doing some cleanup to pass doctests, better test coverage, and lazy importing the catalog.
|
comment:103 Changed 2 years ago by
Thanks Travis.
comment:104 Changed 2 years ago by
- Commit changed from bc8119f90fd742a98010f8f7e7604df2bcae2d96 to ad8ca4de3dcb1b9b217c5363443dbfe113d84a1b
Branch pushed to git repo; I updated commit sha1. New commits:
ad8ca4d | Fixing the docbuild by having the correct files listed.
|
comment:105 Changed 2 years ago by
Green patchbot.
comment:106 Changed 2 years ago by
So are my changes good? Is this a positive review?
comment:107 Changed 2 years ago by
- 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
- Commit changed from ad8ca4de3dcb1b9b217c5363443dbfe113d84a1b to 9d3a838478a8c7f218bf333c2a58f598c2fe7f8b
Branch pushed to git repo; I updated commit sha1. New commits:
b248c6a | Changed to one line if
|
79192f9 | Merge branch 'public/combinat/path_tableaux-25434' of git://trac.sagemath.org/sage into t/25434/pathtableaux
|
29732eb | Added _an_element_
|
9d3a838 | Merge branch 'public/combinat/path_tableaux-25434' of git://trac.sagemath.org/sage into t/25434/pathtableaux
|
comment:109 Changed 2 years ago by
- Status changed from needs_work to needs_review
comment:111 Changed 2 years ago by
- Branch changed from public/combinat/path_tableaux-25434 to 9d3a838478a8c7f218bf333c2a58f598c2fe7f8b
- Resolution set to fixed
- Status changed from positive_review to closed
I have been unable to push my commits. I get Permission Denied. I will try again in the morning.