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)
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
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
Sounds good!
While we are at it, what about using the following idiom:
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 :-)