Ticket #12882 (needs_review enhancement)

Opened 14 months ago

Last modified 3 weeks ago

Allows a generalized Cartan matrix as input for Dynkin diagrams

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

Description (last modified by tscrim) (diff)

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

Apply: trac_12882-matrix_as_dynkin_diagram-ts.patch Download

Change History

comment:1 Changed 14 months ago by stumpc5

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

comment:2 Changed 14 months ago by stumpc5

  • Dependencies set to #8327

comment:3 Changed 11 months ago by jdemeyer

Please fill in your real name as Author.

comment:4 Changed 11 months ago by stumpc5

  • Authors set to Christian Stump

comment:5 follow-up: ↓ 8 Changed 7 months 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 7 months ago by hthomas

Hi Christian--

Why does this have #8327 listed as a dependency?

cheers,

Hugh

comment:7 Changed 7 months ago by stumpc5

  • Dependencies #8327 deleted

comment:8 in reply to: ↑ 5 Changed 7 months 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

Changed 7 months ago by stumpc5

comment:9 Changed 7 months 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 2 months ago by tscrim

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

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

Changed 2 months ago by tscrim

comment:11 Changed 2 months ago by tscrim

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

New version of the patch ready for review.

For patchbot:

Apply: trac_12882-matrix_as_dynkin_diagram-ts.patch

Changed 3 weeks ago by tscrim

comment:12 Changed 3 weeks ago by tscrim

Rebased patch.

Apply: trac_12882-matrix_as_dynkin_diagram-ts.patch

Note: See TracTickets for help on using tickets.