Opened 8 years ago

Closed 8 years ago

#14655 closed defect (fixed)

Dynkin diagrams does not copy like digraph

Reported by: tscrim Owned by: sage-combinat
Priority: major Milestone: sage-5.11
Component: combinatorics Keywords: dynkin diagram copy
Cc: sage-combinat, nthiery, mshimo, aschilling Merged in: sage-5.11.beta1
Authors: Travis Scrimshaw Reviewers: Mark Shimozono
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

Initializing:

sage: d = DynkinDiagram(['B',3,1])
sage: g = DiGraph()
sage: for x in d.vertices(): g.add_vertex(x)
sage: for y in d.edges(): g.add_edge(y)
sage: cd = copy(d)
sage: cg = copy(g)
sage: cd is d
False
sage: cd == d and cg == g
True

Now the correct behavior using DiGraph:

sage: g.vertices()
[0, 1, 2, 3]
sage: cg.vertices()
[0, 1, 2, 3]
sage: cg.add_vertex(4)
sage: g.vertices()
[0, 1, 2, 3]
sage: cg.vertices()
[0, 1, 2, 3, 4]

The error with DynkinDiagram:

sage: d.vertices()
[0, 1, 2, 3]
sage: cd.vertices()
[0, 1, 2, 3]
sage: cd.add_vertex(4)       
sage: cd.vertices()
[0, 1, 2, 3, 4]
sage: d.vertices()
[0, 1, 2, 3, 4]

This causes problems with automorphism_group(edge_labels=True) noticed by Mark Shimozono.

Attachments (1)

trac_14655-dynkin_copy-ts.patch (4.3 KB) - added by tscrim 8 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 8 years ago by tscrim

  • Cc aschilling added

comment:2 Changed 8 years ago by tscrim

  • Authors set to Travis Scrimshaw
  • Status changed from new to needs_review

Fixed by making DynkinDiagram_class.__init__() handle the input from GenericGraph.__copy__() and do some type-checking.

comment:3 Changed 8 years ago by mshimo

  • Reviewers set to mshimo
  • Status changed from needs_review to needs_work

Thanks, Travis, for fixing this. A few small things to deal with:

Please fix the docstring for :func:DynkinDiagram: the real issue for the Cartan matrix convention is transposing or not; in everyone's conventions the arrow points from a *longer* root to a *shorter* one.

I ran the following tests and got some errors.

sage: d = DynkinDiagram(['A',3,1])
sage: TestSuite(d).run()
Failure in _test_not_implemented_methods:
Traceback (click to the left of this block for traceback)
...
The following tests failed: _test_not_implemented_methods

I'm not sure I understand why, but :class:DynkinDiagram_class inherits from :class:CartanType_abstract and therefore is obligated to supply its abstract methods. It appears not to fulfill its promise for :meth:is_finite, :meth:is_affine, and :meth:is_irreducible.

Otherwise it looks great!

comment:4 Changed 8 years ago by tscrim

  • Reviewers changed from mshimo to Mark Shimozono
  • Status changed from needs_work to needs_review

I believe it is so that you can easily pass Dynkin diagrams as Cartan types (without using the coercion system). Anyways, new version of the patch which implements the additional methods and makes the docstring fix.

comment:5 Changed 8 years ago by mshimo

  • Status changed from needs_review to positive_review

comment:6 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.10 to sage-5.11

comment:7 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_info

The patch file is missing some metadata, in particular a username. Did you create the patch using hg export?

Changed 8 years ago by tscrim

comment:8 Changed 8 years ago by tscrim

  • Status changed from needs_info to needs_review

Forgot to export. Fixed.

comment:9 Changed 8 years ago by tscrim

  • Status changed from needs_review to positive_review

comment:10 Changed 8 years ago by jdemeyer

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