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 height() and width() functions to square grid paths #8353

Closed
sagetrac-abmasse mannequin opened this issue Feb 24, 2010 · 15 comments
Closed

Adding height() and width() functions to square grid paths #8353

sagetrac-abmasse mannequin opened this issue Feb 24, 2010 · 15 comments

Comments

@sagetrac-abmasse
Copy link
Mannequin

sagetrac-abmasse mannequin commented Feb 24, 2010

When dealing with 2d word paths, it is very useful to know their height and their width.

In particular, one can compute a bounding box for a better displaying. The aim of this small ticket is to add those two functionalities.

By the way, while testing it, I noticed a strange behavior:

sage: Paths = WordPaths('abcABC')
sage: p = Paths('acABC')
sage: points = list(p.points())
sage: points
[(0, 0), (1, 0), (1/2, 1/2*sqrt3), (-1/2, 1/2*sqrt3), (-1, 0), (-1/2, -1/2*sqrt3)]
sage: y_coords = map(lambda (_,y):y, points)
sage: y_coords
[0, 0, 1/2*sqrt3, 1/2*sqrt3, 0, -1/2*sqrt3]
sage: max(y_coords)
-1/2*sqrt3

Shouldn't 1/2*sqrt3 be the highest element ? This doesn't make sense.

CC: @seblabbe @sagetrac-sage-combinat

Component: combinatorics

Keywords: paths, height, width

Author: Alexandre Blondin Massé

Reviewer: Sébastien Labbé

Merged: sage-4.3.4.alpha1

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

@sagetrac-abmasse

This comment has been minimized.

@sagetrac-abmasse

This comment has been minimized.

@sagetrac-abmasse
Copy link
Mannequin Author

sagetrac-abmasse mannequin commented Feb 25, 2010

comment:3

Needs review !

@sagetrac-abmasse

This comment has been minimized.

@sagetrac-abmasse

This comment has been minimized.

@seblabbe
Copy link
Contributor

seblabbe commented Mar 1, 2010

comment:6

I would suggest that the value be converted to RR before taking the max or min. You could add a comment explaining the reason of the conversion and that this bug is trac at a ticket number that you give.

The description of this ticket is not really the good place to put your comment. You can put it in a comment of the ticket instead or on sage-devel. I asked the question on sage-devel :
http://groups.google.com/group/sage-devel/browse_thread/thread/2557a48ad695b42e

@sagetrac-abmasse
Copy link
Mannequin Author

sagetrac-abmasse mannequin commented Mar 1, 2010

comment:7

Thanks for your comments !

I found an almost-clean solution for the problem raised by the triangular grid paths. Instead of computing directly the height() and width() functions, I introduced the xmin(), xmax(), ymin() and ymax() functions we talked about. They are called by the two first functions. To solve the problem for the triangular grid paths, I just redefined them by coercing the x- and y-coordinates to real values, so that the max and min functions be well-defined.

I'll upload another patch in a few minutes. Needs review !

@sagetrac-abmasse
Copy link
Mannequin Author

sagetrac-abmasse mannequin commented Mar 1, 2010

New patch -- adds functions xmin(), xmax(), etc.

@sagetrac-abmasse
Copy link
Mannequin Author

sagetrac-abmasse mannequin commented Mar 1, 2010

@seblabbe
Copy link
Contributor

seblabbe commented Mar 3, 2010

Attachment: trac_8353_review-sl.patch.gz

Applies over the precedent patch

@seblabbe
Copy link
Contributor

seblabbe commented Mar 3, 2010

Author: Alexandre Blondin Massé

@seblabbe
Copy link
Contributor

seblabbe commented Mar 3, 2010

comment:9

All tests passed in sage/combinat/words. Documentation builds fine. The issue discussed above is fixed. I am giving a positive review to Alex's changes.

I added a small patch fixing some documentation. If Alexandre agrees with the changes I did, I allow him to change the status of the ticket to positive review.

@seblabbe
Copy link
Contributor

seblabbe commented Mar 3, 2010

Reviewer: Sébastien Labbé

@sagetrac-abmasse
Copy link
Mannequin Author

sagetrac-abmasse mannequin commented Mar 3, 2010

comment:10

I agree with Sébastien's changes. I retested it just to make sure, and I looked at the documentation, which is still fine. I'll set this ticket to "positive review".

@mwhansen
Copy link
Contributor

mwhansen commented Mar 6, 2010

Merged: sage-4.3.4.alpha1

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