Opened 6 months ago

Closed 6 months ago

#15277 closed defect (fixed)

CartanMatrix to DynkinDiagram adds edges with zeros

Reported by: tscrim Owned by: sage-combinat
Priority: major Milestone: sage-5.13
Component: combinatorics Keywords: Cartan matrix Dynkin diagram
Cc: sage-combinat, aschilling, nthiery Merged in: sage-5.13.beta1
Authors: Travis Scrimshaw Reviewers: Nicolas M. Thiéry, Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by tscrim)

Currently we have:

sage: CM = CartanMatrix([[2,-1,0,0],[-3,2,-2,-2],[0,-1,2,-1],[0,-1,-1,2]])
sage: CM
[ 2 -1  0  0]
[-3  2 -2 -2]
[ 0 -1  2 -1]
[ 0 -1 -1  2]
sage: CM.dynkin_diagram().edges()
[(0, 1, 3),
 (0, 2, 0),
 (0, 3, 0),
 (1, 0, 1),
 (1, 2, 1),
 (1, 3, 1),
 (2, 0, 0),
 (2, 1, 2),
 (2, 3, 1),
 (3, 0, 0),
 (3, 1, 2),
 (3, 2, 1)]

The edges with the label of 0 should not be included.

Attachments (2)

trac_15277-dynkin_diagram_fix-ts.patch (1.7 KB) - added by tscrim 6 months ago.
trac_15277_review.patch (1.7 KB) - added by chapoton 6 months ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 6 months ago by tscrim

  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 6 months ago by nthiery

Sounds good!

While we are at it, what about using the following idiom:

    for (i,j) in M.nonzero_positions():
        ...

Granted, exploiting sparseness here is essentially an eye candy: we are not really likely to compute the dynkin diagrams of many large sparse cartan matrices where this would actually do a difference timewise. But still an eye candy :-)

Changed 6 months ago by tscrim

comment:3 Changed 6 months ago by tscrim

Done.

Changed 6 months ago by chapoton

comment:4 Changed 6 months ago by chapoton

Hello,

Looks good to me. I have added a small review patch.

1) removing an unused import of sgn

2) computing the value of ct, where it was needed

3) removing the variable n

If you agree with these details (found with pyflakes), you can set positive review

comment:5 Changed 6 months ago by tscrim

I'm happy with it. Nicolas?

comment:6 Changed 6 months ago by nthiery

The changes to please pyflakes are not directly related with this ticket; but if there is no ongoing ticket which conflicts with this one, this occasion is as good as another. Thanks Travis and Frederic.

comment:7 Changed 6 months ago by tscrim

  • Reviewers set to Nicolas Thiery, Frederic Chapoton
  • Status changed from needs_review to positive_review

This is the only ticket I know of that is changing dynkin_diagram.py, so I'm going to set this to positive review. Thanks Nicolas and Frederic.

comment:8 Changed 6 months ago by chapoton

  • Reviewers changed from Nicolas Thiery, Frederic Chapoton to Nicolas Thiery, Frédéric Chapoton

comment:9 Changed 6 months ago by nthiery

  • Reviewers changed from Nicolas Thiery, Frédéric Chapoton to Nicolas M. Thiéry, Frédéric Chapoton

comment:10 Changed 6 months ago by jdemeyer

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