Allows a generalized Cartan matrix as input for Dynkin diagrams

[stumpc5]

Dynkin diagrams can currently not be constructed using a generalized Cartan matrix.

Apply: attachment: trac_12882-matrix_as_dynkin_diagram-ts.patch

Depends on #14137
Depends on #14516

CC: @sagetrac-sage-combinat

Component: combinatorics

Keywords: Weyl group, Dynkin diagram, Cartan matrix, days49

Author: Christian Stump, Travis Scrimshaw

Reviewer: Ben Salisbury

Merged: sage-5.12.beta4

Issue created by migration from https://trac.sagemath.org/ticket/12882

[stumpc5]

Dependencies: #8327

[jdemeyer]

comment:3

Please fill in your real name as Author.

[stumpc5]

Author: Christian Stump

[hughrthomas]

comment:5

Hi Christian--

Your code is checking that all the off-diagonal entries in the input matrix are strictly negative. That seems a bit over-enthusiastic.

A doctest for the new functionality would also be good.

cheers,

Hugh

[hughrthomas]

comment:6

Hi Christian--

Why does this have #8327 listed as a dependency?

cheers,

Hugh

[stumpc5]

Changed dependencies from #8327 to none

[stumpc5]

comment:8

Replying to @hughrthomas:

Hi Christian--

Your code is checking that all the off-diagonal entries in the input matrix are strictly negative. That seems a bit over-enthusiastic.

A doctest for the new functionality would also be good.

Hi Hugh!

Thanks for bringing back my/our attention to this ticket!

I will be too busy to actively work on this patch in the next two weeks, but feel free to take it over and review/finish it if you like! I will now provide a version that you can work with (in particular, I deleted the dependency). Of course, you can as well wait until I finish the patch, if you prefer.

Best, Christian

[stumpc5]

Attachment: trac_12882-generalized_cartan_matrix_as_dynkin_diagram_input-cs.patch.gz

[stumpc5]

comment:9

I updated the patch on the combinat queue and also uploaded it here. The negative entries test was accidentally fixed in the following patch on reflection groups rather than in this patch. I replaced all asserts by ValueErrors (since I was told that's the way to do it some time ago).

As said, Hugh: feel free to take it over, or to review it as is.

Best, Christian

[tscrim]

comment:10

I will take this over (since no progress has been made recently) and base it on #14137.

[tscrim]

Dependencies: #14137

[tscrim]

Changed author from Christian Stump to Christian Stump, Travis Scrimshaw

[tscrim]

Attachment: trac_12882-generalized_cartan_matrix_as_dynkin_diagram_input-cs.2.patch.gz

[tscrim]

comment:11

New version of the patch ready for review.

For patchbot:

Apply: trac_12882-matrix_as_dynkin_diagram-ts.patch

[tscrim]

comment:12

Rebased patch.

Apply: trac_12882-matrix_as_dynkin_diagram-ts.patch

[bsalisbury1]

comment:13

Hi Travis,

This patch does not pass all of it's doctests. Also, is it a good idea to add a note to the documentation saying that only Cartan matrices whose Cartan types are implemented in Sage are legal input? For example, the Cartan matrix

CartanMatrix([[2,-3],[-3,2]])

yields an error. Can you also provide an example of inputting a Cartan matrix and an index set in the documentation?

-Ben

[bsalisbury1]

Changed keywords from Weyl group, Dynkin diagram, Cartan matrix to Weyl group, Dynkin diagram, Cartan matrix, days49

[tscrim]

comment:15

Hey Ben,

New version of the patch which passes all doctests and adds the example. Thanks for reviewing this.

Best,

Travis

For patchbot:

Apply: trac_12882-matrix_as_dynkin_diagram-ts.patch

[tscrim]

comment:21

Ah, there's a dependency from #14516 for those doctests. Ben and I were doing this on the combinat queue which had that patch applied.

sage -t root_system/dynkin_diagram.py
    [88 tests, 3.67 s]
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 4.1 seconds
    cpu time: 1.8 seconds
    cumulative wall time: 3.7 seconds

For patchbot:

Apply: trac_12882-matrix_as_dynkin_diagram-ts.patch​

[tscrim]

Changed dependencies from #14137 to #14137 #14516

[jdemeyer]

comment:22

sage -t devel/sage/sage/combinat/root_system/dynkin_diagram.py
**********************************************************************
File "devel/sage/sage/combinat/root_system/dynkin_diagram.py", line 149, in sage.combinat.root_system.dynkin_diagram.DynkinDiagram
Failed example:
    DII.index_set()
Expected:
    ('x', -1)
Got:
    (-1, 'x')
**********************************************************************

[tscrim]

comment:23

Okay, I've made the offending test return a sorted list. However this should be overkill since index_set just makes the call to vertices, which returns a sorted list, into a tuple (i.e. there doesn't seem to be a set/dict being returned...). Here's the patches have applied on 5.11.rc0:

trac_14882-backtrack_longtime-dg.patch
trac_13589-categories-c3_under_control-nt.patch
trac_14507-tropical_semiring-ts.patch
trac_7983-major_index_and_other_tableau_fixes-dg.patch
trac_7983-review-ts.patch
trac_8386_really_just_moving-fc.patch
trac_8386_big_clean_fc.patch
trac_8386_assert_removal.patch
trac_14808-recoils_of_permutations-ts.patch
trac_10630-vector_partition-ap.patch
trac_14870-fix_int_mod_QQ-ts.patch
trac_14787-gyw_stats-bs.patch
trac_14787-pdf_fix-ts.patch
trac_14573-path_realizations-ts.patch
trac_11407-list_clone_improve-fh.patch
trac_14516-crystals_speedup-ts.patch
trac_12882-matrix_as_dynkin_diagram-ts.patch

So I'm confused about why we're getting different results (and slightly worried).

Anyways, Ben could you do a quick re-review of the patch? Thanks.

For patchbot:

Apply: trac_12882-matrix_as_dynkin_diagram-ts.patch​

[bsalisbury1]

comment:24

I ran the tests again on 5.12.beta0 on the entire root_system folder with following patches applied:

applying trac_14610-LSenergy-ms.patch
applying trac9107_nesting_nested_classes.patch
applying trac_9107_fix_cross_reference.patch
applying trac_14787-gyw_stats-bs.patch
applying trac_14787-pdf_fix-ts.patch
applying trac_14573-path_realizations-ts.patch
applying trac_11407-list_clone_improve-fh.patch
applying trac_14516-crystals_speedup-ts.patch
applying trac_12882-matrix_as_dynkin_diagram-ts.patch

All tests passed

----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 235.7 seconds
    cpu time: 212.6 seconds
    cumulative wall time: 233.4 seconds

and in particular:

sage -t --long root_system/dynkin_diagram.py
    [88 tests, 0.53 s]

I also copied and ran those tests in Sage directly, and received the expected result:

sage: CII = CartanMatrix([[2, -3], [-4, 2]])
sage: DII = DynkinDiagram(CII, (-1, 'x'))
sage: DII.index_set()
('x', -1)

[jdemeyer]

comment:26

Still doesn't work:

sage -t devel/sage/sage/combinat/root_system/dynkin_diagram.py
**********************************************************************
File "devel/sage/sage/combinat/root_system/dynkin_diagram.py", line 149, in sage.combinat.root_system.dynkin_diagram.DynkinDiagram
Failed example:
    sorted(DII.index_set())
Expected:
    ['x', -1]
Got:
    [-1, 'x']
**********************************************************************

[tscrim]

comment:27

Jeroen, so somehow your system (and the patchbot's) is sorting things differently than ours. Do you have any idea about why this is happening? I don't want to change the output of the doctest because then it will fail on our computers. (My guess as to why we're only seeing this here because it's the only time we're doctesting a sorted list with a string and an integer.)

[jdemeyer]

comment:28

Replying to @tscrim:

My guess as to why we're only seeing this here because it's the only time we're doctesting a sorted list with a string and an integer.

Yes, it seems that comparing a string and an integer simply isn't well-defined.

[tscrim]

comment:29

Attachment: trac_12882-matrix_as_dynkin_diagram-ts.patch.gz

Here's a new version of the patch which sidesteps the issue by making it indexed by two strings. Jeroen, could you check to make sure the tests also pass for you? Thanks.

[fchapoton]

comment:30

apply only trac_12882-matrix_as_dynkin_diagram-ts.patch​

[fchapoton]

comment:31

apply trac_12882-matrix_as_dynkin_diagram-ts.patch

[tscrim]

comment:32

Hey Ben,

Could you check to make sure this works for you still? I've given the patchbot a kick and we'll check against that going green.

Best,

Travis

[fchapoton]

comment:33

it seems that the patchbot is (or was?) broken (it tries to apply very old dependencies)

[bsalisbury1]

comment:34

All tests passed for me.

[jdemeyer]

Merged: sage-5.12.beta4