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 stumpc5)

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)

trac_14123-binary-trees-maps-rebase-cs.patch (14.2 KB) - added by stumpc5 7 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 7 years ago by VivianePons

  • Status changed from new to needs_review

apply trac_14123-binary-trees-maps-vp.patch

comment:2 Changed 7 years ago by stumpc5

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 chrisjamesberg

  • Status changed from needs_review to needs_work

Needs to be rebased with finished #8703.

comment:4 Changed 7 years ago by stumpc5

  • Description modified (diff)
  • Reviewers set to Christian Stump
  • Status changed from needs_work to needs_review

comment:5 Changed 7 years ago by stumpc5

  • Description modified (diff)

comment:6 Changed 7 years ago by stumpc5

If no one else has objections, I'd give it a positive review.

comment:7 Changed 7 years ago by VivianePons

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 stumpc5

Isn't this (part of) the content of #14302?

Last edited 7 years ago by stumpc5 (previous) (diff)

comment:9 Changed 7 years ago by stumpc5

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.
  • 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

Last edited 7 years ago by stumpc5 (previous) (diff)

comment:10 Changed 7 years ago by VivianePons

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 stumpc5

  • Status changed from needs_review to positive_review

comment:12 Changed 7 years ago by jdemeyer

  • 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 stumpc5

@Viviane, are you taking care of that, or should I do it?

comment:14 Changed 7 years ago by VivianePons

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 stumpc5

comment:15 Changed 7 years ago by stumpc5

  • Status changed from needs_work to positive_review

comment:16 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.10.rc0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.