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

Cleanup of sage.graphs.pq_trees #17804

Closed
nathanncohen mannequin opened this issue Feb 18, 2015 · 9 comments
Closed

Cleanup of sage.graphs.pq_trees #17804

nathanncohen mannequin opened this issue Feb 18, 2015 · 9 comments

Comments

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Feb 18, 2015

While fixing the bug reported at #17787, I noticed several things in sage.graphs.pq_trees that should be cleaned. Some misnamed functions, hard-to-read documentation, and also a couple of simple but useful missing features that can be very helpful when debugging code.

This branch consists of several commits which do the following:

  • Rename .cardinality to .number_of_children: the PQ-trees encode a set of permutations, and the 'cardinality' function should represent that, instead of what it represents now.

  • Add a real .cardinality function, which can be used to compute the number of different representations of an interval graph

  • Add a .orderings function, which lists all possibles representations of an interval graph

  • remove .is_P and .is_Q. These functions were barely used in the code itself, and can be replaced with isinstance(x,P) and isinstance(x,Q).

  • Move the documentation of class PQ-tree, which actually explains how the main algorithm works, into the module's doc. It is also rewritten, and hopefully easier to understand.

  • Some one-line changes that improve readability or add links.

Nathann

CC: @dcoudert @dimpase

Component: graph theory

Author: Nathann Cohen

Branch/Commit: 07ca59d

Reviewer: David Coudert

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

@nathanncohen nathanncohen mannequin added this to the sage-6.6 milestone Feb 18, 2015
@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Feb 18, 2015

Branch: u/ncohen/17804

@nathanncohen nathanncohen mannequin added the s: needs review label Feb 18, 2015
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 18, 2015

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

707124ctrac #17804: Rename .cardinality to .number_of_children
6a1b563trac #17804: Add cardinality function to know the number of orderings
7fa50a0trac #17804: Add orderings() to list all orderings
7f73ceatrac #17804: Remove .is_P and .is_Q
ff247cetrac #17804: Move and rephrase the PQ-trees documentation at module's level
07ca59dtrac #17804: Typos and one-line changes

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 18, 2015

Commit: 07ca59d

@dcoudert
Copy link
Contributor

comment:4

Hello,

the patch is good (install, docbuild html and pdf, tests).

Before the 4th item (One at a time, we update the data structure...), it would be nice to have some space. I don't know how to force it (I tried).

Also, if you want to play more with your new PLOT command, you could add an example of PQ-tree ;)

David.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Feb 20, 2015

comment:5

Hellooooooooooooooo,

Before the 4th item (One at a time, we update the data structure...), it would be nice to have some space. I don't know how to force it (I tried).

I do not know either. That is because Sphinx does not like it when a list ends with a sublist. I met this bug several times but I do not know any clean workaround :-/

Also, if you want to play more with your new PLOT command, you could add an example of PQ-tree ;)

I did try, but decided against it because all the pictures I was able to produce took the whole screen vertically, and really 'broke the flow' of the explanations. So I decided against it. This is the kind of options of "plot" that we will need very soon :-P

Nathann

@dcoudert
Copy link
Contributor

comment:7

So let the text as it is.
David.

@dcoudert
Copy link
Contributor

Reviewer: David Coudert

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Feb 20, 2015

comment:9

Thanks !

@vbraun
Copy link
Member

vbraun commented Feb 21, 2015

Changed branch from u/ncohen/17804 to 07ca59d

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

2 participants