Ticket #8353 (closed enhancement: fixed)

Opened 3 years ago

Last modified 3 years ago

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

Reported by: abmasse Owned by: abmasse
Priority: major Milestone: sage-4.3.4
Component: combinatorics Keywords: paths, height, width
Cc: slabbe, sage-combinat Work issues:
Report Upstream: N/A Reviewers: Sébastien Labbé
Authors: Alexandre Blondin Massé Merged in: sage-4.3.4.alpha1
Dependencies: Stopgaps:

Description (last modified by abmasse) (diff)

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.

Attachments

trac_8353_square_grid_word_paths_height_width-abm.patch Download (8.5 KB) - added by abmasse 3 years ago.
New patch -- adds functions xmin(), xmax(), etc.
trac_8353_review-sl.patch Download (1.6 KB) - added by slabbe 3 years ago.
Applies over the precedent patch

Change History

comment:1 Changed 3 years ago by abmasse

  • Description modified (diff)

comment:2 Changed 3 years ago by abmasse

  • Description modified (diff)

comment:3 Changed 3 years ago by abmasse

  • Status changed from new to needs_review

Needs review !

comment:4 Changed 3 years ago by abmasse

  • Description modified (diff)

comment:5 Changed 3 years ago by abmasse

  • Description modified (diff)

comment:6 Changed 3 years ago by slabbe

  • Status changed from needs_review to needs_work

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

comment:7 Changed 3 years ago by abmasse

  • Owner changed from sage-combinat to abmasse

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 !

Changed 3 years ago by abmasse

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

comment:8 Changed 3 years ago by abmasse

  • Status changed from needs_work to needs_review

Changed 3 years ago by slabbe

Applies over the precedent patch

comment:9 Changed 3 years ago by slabbe

  • Reviewers set to Sébastien Labbé
  • Authors set to Alexandre Blondin Massé

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.

comment:10 Changed 3 years ago by abmasse

  • Status changed from needs_review to positive_review

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

comment:11 Changed 3 years ago by mhansen

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