Opened 8 years ago

Closed 8 years ago

# CartanMatrix to DynkinDiagram adds edges with zeros

Reported by: Owned by: tscrim sage-combinat major sage-5.13 combinatorics Cartan matrix Dynkin diagram sage-combinat, aschilling, nthiery sage-5.13.beta1 Travis Scrimshaw Nicolas M. Thiéry, Frédéric Chapoton N/A

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.

### comment:1 Changed 8 years ago by tscrim

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

### comment:2 Changed 8 years 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 :-)

Done.

### comment:4 Changed 8 years 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 8 years ago by tscrim

I'm happy with it. Nicolas?

### comment:6 Changed 8 years 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 8 years 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 8 years ago by chapoton

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

### comment:9 Changed 8 years ago by nthiery

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

### comment:10 Changed 8 years 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.