Opened 6 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 )
Dynkin diagrams can currently not be constructed using a generalized Cartan matrix.
Attachments (3)
Change History (38)
comment:1 Changed 6 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:2 Changed 6 years ago by
- Dependencies set to #8327
comment:3 Changed 5 years ago by
comment:4 Changed 5 years ago by
comment:5 follow-up: ↓ 8 Changed 5 years ago by
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 5 years ago by
comment:7 Changed 5 years ago by
- Dependencies #8327 deleted
comment:8 in reply to: ↑ 5 Changed 5 years ago by
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
Changed 5 years ago by
comment:9 Changed 5 years ago by
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 5 years ago by
- 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.
Changed 5 years ago by
comment:11 Changed 5 years ago by
- 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 5 years ago by
Rebased patch.
Apply: trac_12882-matrix_as_dynkin_diagram-ts.patch
comment:13 Changed 4 years ago by
- 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
- Cc days49 removed
- Keywords days49 added
comment:15 Changed 4 years ago by
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
Hey Ben,
Okay, is_finite()
and is_affine()
are working now for arbitrary Cartan matrices.
Best,
Travis
comment:17 Changed 4 years ago by
For patchbot:
Apply: trac_12882-matrix_as_dynkin_diagram-ts.patch
comment:18 Changed 4 years ago by
- 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
- Milestone changed from sage-5.11 to sage-5.12
comment:20 Changed 4 years ago by
- 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
- 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
- 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
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
comment:24 Changed 4 years ago by
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
- Status changed from needs_work to positive_review
comment:26 Changed 4 years ago by
- 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: ↓ 28 Changed 4 years ago by
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
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.
Changed 4 years ago by
comment:29 Changed 4 years ago by
- 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
apply only trac_12882-matrix_as_dynkin_diagram-ts.patch
comment:31 Changed 4 years ago by
apply trac_12882-matrix_as_dynkin_diagram-ts.patch
comment:32 Changed 4 years ago by
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
it seems that the patchbot is (or was?) broken (it tries to apply very old dependencies)
comment:34 Changed 4 years ago by
- Status changed from needs_review to positive_review
All tests passed for me.
comment:35 Changed 4 years ago by
- Merged in set to sage-5.12.beta4
- Resolution set to fixed
- Status changed from positive_review to closed
Please fill in your real name as Author.