Opened 6 months ago

Closed 6 months 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 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 :-)

Done.

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.