Opened 5 years ago

Closed 4 years ago

#12882 closed enhancement (fixed)

Allows a generalized Cartan matrix as input for Dynkin diagrams

Reported by: stumpc5 Owned by: sage-combinat
Priority: major Milestone: sage-5.12
Component: combinatorics Keywords: Weyl group, Dynkin diagram, Cartan matrix, days49
Cc: sage-combinat Merged in: sage-5.12.beta4
Authors: Christian Stump, Travis Scrimshaw Reviewers: Ben Salisbury
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #14137 #14516 Stopgaps:

Description (last modified by tscrim)

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

Apply: trac_12882-matrix_as_dynkin_diagram-ts.patch

Change History (38)

comment:1 Changed 5 years ago by stumpc5

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

comment:2 Changed 5 years ago by stumpc5

  • Dependencies set to #8327

comment:3 Changed 5 years ago by jdemeyer

Please fill in your real name as Author.

comment:4 Changed 5 years ago by stumpc5

  • Authors set to Christian Stump

comment:5 follow-up: Changed 4 years ago by hthomas

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

comment:6 Changed 4 years ago by hthomas

Hi Christian--

Why does this have #8327 listed as a dependency?

cheers,

Hugh

comment:7 Changed 4 years ago by stumpc5

  • Dependencies #8327 deleted

comment:8 in reply to: ↑ 5 Changed 4 years ago by stumpc5

Replying to hthomas:

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

comment:9 Changed 4 years ago by stumpc5

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

comment:10 Changed 4 years ago by tscrim

  • Authors changed from Christian Stump to Christian Stump, Travis Scrimshaw
  • Cc sage-combinat added
  • Dependencies set to #14137
  • Status changed from needs_review to needs_work

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

comment:11 Changed 4 years ago by tscrim

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

New version of the patch ready for review.

For patchbot:

Apply: trac_12882-matrix_as_dynkin_diagram-ts.patch

comment:12 Changed 4 years ago by tscrim

Rebased patch.

Apply: trac_12882-matrix_as_dynkin_diagram-ts.patch

comment:13 Changed 4 years ago by bsalisbury1

  • Cc days49 added

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

comment:14 Changed 4 years ago by bsalisbury1

  • Cc days49 removed
  • Keywords days49 added

comment:15 Changed 4 years ago by tscrim

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

comment:16 Changed 4 years ago by tscrim

Hey Ben,

Okay, is_finite() and is_affine() are working now for arbitrary Cartan matrices.

Best,
Travis

comment:17 Changed 4 years ago by tscrim

For patchbot:

Apply: trac_12882-matrix_as_dynkin_diagram-ts.patch

comment:18 Changed 4 years ago by bsalisbury1

  • Reviewers set to Ben Salisbury
  • Status changed from needs_review to positive_review

Hi Travis,

Thank you for making those changes!

-Ben

comment:19 Changed 4 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:20 Changed 4 years ago by jdemeyer

  • Status changed from positive_review to needs_work

There are obvious doctest failures with the patchbot:

sage -t devel/sage/sage/combinat/root_system/dynkin_diagram.py
**********************************************************************
File "devel/sage/sage/combinat/root_system/dynkin_diagram.py", line 145, in sage.combinat.root_system.dynkin_diagram.DynkinDiagram
Failed example:
    DI.index_set()
Expected:
    (3, 5)
Got:
    [3, 5]
**********************************************************************
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']
**********************************************************************

comment:21 Changed 4 years ago by tscrim

  • Dependencies changed from #14137 to #14137 #14516
  • Status changed from needs_work to positive_review

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​

comment:22 Changed 4 years ago by jdemeyer

  • Status changed from positive_review to needs_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:
    DII.index_set()
Expected:
    ('x', -1)
Got:
    (-1, 'x')
**********************************************************************

comment:23 Changed 4 years ago by tscrim

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​

Last edited 4 years ago by tscrim (previous) (diff)

comment:24 Changed 4 years ago by bsalisbury1

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)

comment:25 Changed 4 years ago by bsalisbury1

  • Status changed from needs_work to positive_review

comment:26 Changed 4 years ago by jdemeyer

  • Status changed from positive_review to needs_work

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']
**********************************************************************

comment:27 follow-up: Changed 4 years ago by tscrim

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.)

comment:28 in reply to: ↑ 27 Changed 4 years ago by jdemeyer

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.

comment:29 Changed 4 years ago by tscrim

  • Status changed from needs_work to needs_review

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.

comment:30 Changed 4 years ago by chapoton

apply only trac_12882-matrix_as_dynkin_diagram-ts.patch​

comment:31 Changed 4 years ago by chapoton

apply trac_12882-matrix_as_dynkin_diagram-ts.patch

comment:32 Changed 4 years ago by tscrim

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

comment:33 Changed 4 years ago by chapoton

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

comment:34 Changed 4 years ago by bsalisbury1

  • Status changed from needs_review to positive_review

All tests passed for me.

comment:35 Changed 4 years ago by jdemeyer

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