Ticket #2023 (closed enhancement: fixed)

Opened 5 years ago

Last modified 8 days ago

Latex pictures for Dynkin diagram and misc improvements to Cartan types

Reported by: wdj Owned by: mhansen
Priority: minor Milestone: sage-5.10
Component: combinatorics Keywords:
Cc: sage-combinat Work issues:
Report Upstream: N/A Reviewers: Nicolas M. Thiéry
Authors: Travis Scrimshaw Merged in: sage-5.10.beta3
Dependencies: #13605 #14085 #14248 Stopgaps:

Description (last modified by nthiery) (diff)

Displaying nice latex pictures for all Dynkin diagrams of affine and classical types.

Also:

  • Improves the description of which conventions are used for Dynkin diagrams and Cartan matrices (Bourbaki/Kac?).
  • Misc improvements to Cartan types, including a global option for displaying Cartan types using Kac's conventions and much documentation cleanup.

Attachments

trac_2023-dynkin_graphs-ts.patch Download (121.0 KB) - added by nthiery 13 days ago.
Minor doc tweak

Change History

comment:1 Changed 5 years ago by mabshoff

  • Milestone changed from sage-2.10.3 to sage-2.10.2

comment:2 Changed 5 years ago by mabshoff

Mike,

this has been sitting around for a while. What is the status here?

Cheers,

Michael

comment:3 Changed 4 years ago by nthiery

  • Cc sage-combinat added

comment:4 Changed 4 years ago by nthiery

It's indeed a bit of an abuse to have Dynkin diagram derive from Digraphs (some edges are not oriented). But they are not Graphs either (some edges are). They don't really deserve a special class graph just for themselves, do they? So, I guess we can live with this abuse.

That being said, plot should definitely be overriden to get appropriate pictures. Volunteers?

See also #5502 for ascii art drawing.

comment:5 Changed 3 months ago by tscrim

  • Status changed from new to needs_review
  • Report Upstream set to N/A
  • Dependencies set to #14085
  • Authors set to Travis Scrimshaw

I've uploaded a patch that gives custom latex printing for Dynkin diagrams for crystallographic types.

comment:6 Changed 2 months ago by tscrim

  • Dependencies changed from #14085 to #13605 #14085 #14248

New version which uses the global options framework for notation choices.

comment:7 Changed 2 months ago by tscrim

Cleaned up some docstrings.

comment:8 Changed 5 weeks ago by nthiery

Hi Travis,

I pushed a reviewer patch on the queue which makes the logic more concise as we had discussed this morning. Please check my changes and fold them. Due to some changes I undid in my patch, you probably want to have a look at the folded patch, and strip away trivial space changes that could be left due to uncomplete undoes.

I'll then have a final check on the updated patch.

Cheers,

Nicolas

comment:9 Changed 5 weeks ago by tscrim

Hey Nicolas,

Thank you for the review. I had to make some minor tweaks to affine types B,C, and D. However this patch will change depending on what happens in #14248.

Thanks,
Travis

comment:10 Changed 5 weeks ago by tscrim

  • Reviewers set to Nicolas M. Thiéry

Updated with a better note about the conventions used in sage.

comment:11 Changed 2 weeks ago by tscrim

  • Description modified (diff)
  • Summary changed from dynkin diagram weights to dynkin diagram latex

comment:12 Changed 2 weeks ago by nthiery

  • Description modified (diff)

comment:13 Changed 2 weeks ago by nthiery

  • Description modified (diff)

I have just been through the patch, and wrote a little reviewer patch which I just pushed to the Sage-Combinat queue. It sounds good to go assuming all tests pass.

Travis: if you are happy with the reviewer patch, please fold upload and set a positive review on my behalf.

Last edited 2 weeks ago by nthiery (previous) (diff)

comment:14 Changed 2 weeks ago by tscrim

  • Status changed from needs_review to positive_review

comment:15 Changed 13 days ago by jdemeyer

  • Status changed from positive_review to needs_work
dochtml.log:[combinat ] /mazur/release/merger/sage-5.10.beta3/devel/sage/doc/en/reference/combinat/sage/combinat/root_system/cartan_type.rst:11: WARNING: error while formatting signature for sage.combinat.root_system.cartan_type.CartanType_crystalographic.ascii_art: invalid syntax (<unknown>, line 1)

comment:16 Changed 13 days ago by nthiery

Sorry, we should have caught that. Worked around in the attached patch. See also #14553.

The updated patch was checked by Travis. I am running the tests now.

comment:17 Changed 13 days ago by nthiery

  • Status changed from needs_work to needs_review

comment:19 Changed 13 days ago by nthiery

  • Status changed from needs_review to positive_review
  • Type changed from defect to enhancement

comment:20 Changed 13 days ago by nthiery

  • Status changed from positive_review to needs_work
  • Work issues set to ignored doctests with the new doctest framework

Some doctests are ignored with the new doctest framework, and the framework complains about it with long tests.

See discussion on:

 https://groups.google.com/forum/?fromgroups=#!topic/sage-devel/4m1ydGdiGf8

comment:21 Changed 13 days ago by nthiery

  • Status changed from needs_work to positive_review
  • Work issues ignored doctests with the new doctest framework deleted

The new patches contains two little changes we agreed with Travis / on sage-devel:

  • Some trailing whitespace in new lines
  • Updating the number of currently ignored tests in doctest/sources

All long test passed.

Back to positive review!

comment:22 Changed 13 days ago by nthiery

  • Summary changed from dynkin diagram latex to Latex pictures for Dynkin diagram and misc improvements to Cartan types

Changed 13 days ago by nthiery

Minor doc tweak

comment:23 Changed 8 days ago by jdemeyer

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