Opened 7 years ago
Closed 7 years ago
#14123 closed enhancement (fixed)
Adding new combinatorial maps to binary trees
Reported by: | VivianePons | Owned by: | sage-combinat |
---|---|---|---|
Priority: | major | Milestone: | sage-5.10 |
Component: | combinatorics | Keywords: | days45 |
Cc: | sage-combinat, stumpc5, chrisjamesberg | Merged in: | sage-5.10.rc0 |
Authors: | Viviane Pons | Reviewers: | Christian Stump |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #8703 | Stopgaps: |
Description (last modified by )
We add different combinatorial maps between binary trees and other combinatorial objects (permutations, dyck path, ...) especially to be used into FindStat?.
Apply only:
Attachments (1)
Change History (17)
comment:1 Changed 7 years ago by
- Status changed from new to needs_review
comment:2 Changed 7 years ago by
Hi Viviane,
could you maybe do a little change in the @combinatorial_map names like in
"recursive map 'L 1 R 0' (Tamari)" -> "recursive map L1R0 (Tamari)"
to make it look a little less scary?
comment:3 Changed 7 years ago by
- Status changed from needs_review to needs_work
Needs to be rebased with finished #8703.
comment:4 Changed 7 years ago by
- Description modified (diff)
- Reviewers set to Christian Stump
- Status changed from needs_work to needs_review
comment:5 Changed 7 years ago by
- Description modified (diff)
comment:6 Changed 7 years ago by
If no one else has objections, I'd give it a positive review.
comment:7 Changed 7 years ago by
The only problem I see is that the names of the map here are the old names, not the new ones we use in Findstat. Chris changed the names on the Findstas patch but not on this one and I didn't take the time to do it either...
comment:8 Changed 7 years ago by
Isn't this (part of) the content of #14302?
comment:9 Changed 7 years ago by
Hi Viviane --
I updated the patch with tons of minor changes (unfortunately, I didn't do it in a separate file to actually show the diff, sorry):
- I removed tons of trailing white spaces, and empty lines
- it's worth looking at the docbuild outcome to see what code is not yet rendering as desired.
- you often tried to use math code, but sphinx wasn't able to figure it out (like in
a string, either `1L0R`,`1R0L`,`L1R0`,`R1L0`
). I replaced these. - when writing rst over multiple lines, please see that the indention is right, I also fixed several itemize going over several lines.
- you often tried to use math code, but sphinx wasn't able to figure it out (like in
- I replaced the assert by a ValueError?.
I hope I fixed everything, but feel free to look over it, and possibly do further changes. Otherwise, I'll give it a positive review as soon as the patchbot is happy.
Cheers, Christian
comment:10 Changed 7 years ago by
Oh ok, I see the name of the maps have been changed on the other ticket.
Thank's a lot for your review and changes. As you said, I hadn't checked the docbuild (my wrong). I've had a look and it seems ok.
comment:11 Changed 7 years ago by
- Status changed from needs_review to positive_review
comment:12 Changed 7 years ago by
- Status changed from positive_review to needs_work
Don't use assertions for bad user input. An assertion failure is by definition a bug in the code. To catch bad user input, use
raise ValueError('foo must be a positive number')
comment:13 Changed 7 years ago by
@Viviane, are you taking care of that, or should I do it?
comment:14 Changed 7 years ago by
If you have time, it would be great if you could do it. I'm a bit overloaded with work right now!
Changed 7 years ago by
comment:15 Changed 7 years ago by
- Status changed from needs_work to positive_review
comment:16 Changed 7 years ago by
- Merged in set to sage-5.10.rc0
- Resolution set to fixed
- Status changed from positive_review to closed
apply trac_14123-binary-trees-maps-vp.patch