Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding new combinatorial maps to binary trees #14123

Closed
VivianePons opened this issue Feb 14, 2013 · 16 comments
Closed

Adding new combinatorial maps to binary trees #14123

VivianePons opened this issue Feb 14, 2013 · 16 comments

Comments

@VivianePons
Copy link
Contributor

We add different combinatorial maps between binary trees and other combinatorial objects (permutations, dyck path, ...) especially to be used into FindStat.

Apply only:

Depends on #8703

CC: @sagetrac-sage-combinat @stumpc5 @sagetrac-chrisjamesberg

Component: combinatorics

Keywords: days45

Author: Viviane Pons

Reviewer: Christian Stump

Merged: sage-5.10.rc0

Issue created by migration from https://trac.sagemath.org/ticket/14123

@VivianePons
Copy link
Contributor Author

comment:1

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

@stumpc5
Copy link
Contributor

stumpc5 commented Mar 13, 2013

comment:2

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?

@sagetrac-chrisjamesberg
Copy link
Mannequin

sagetrac-chrisjamesberg mannequin commented May 6, 2013

comment:3

Needs to be rebased with finished #8703.

@stumpc5

This comment has been minimized.

@stumpc5
Copy link
Contributor

stumpc5 commented May 23, 2013

Reviewer: Christian Stump

@stumpc5

This comment has been minimized.

@stumpc5
Copy link
Contributor

stumpc5 commented May 23, 2013

comment:6

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

@VivianePons
Copy link
Contributor Author

comment:7

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

@stumpc5
Copy link
Contributor

stumpc5 commented May 23, 2013

comment:8

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

@stumpc5
Copy link
Contributor

stumpc5 commented May 24, 2013

comment:9

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

@VivianePons
Copy link
Contributor Author

comment:10

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.

@jdemeyer
Copy link
Contributor

comment:12

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

@stumpc5
Copy link
Contributor

stumpc5 commented May 27, 2013

comment:13

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

@VivianePons
Copy link
Contributor Author

comment:14

If you have time, it would be great if you could do it. I'm a bit overloaded with work right now!

@stumpc5
Copy link
Contributor

stumpc5 commented May 28, 2013

comment:15

Attachment: trac_14123-binary-trees-maps-rebase-cs.patch.gz

@jdemeyer
Copy link
Contributor

Merged: sage-5.10.rc0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants