Opened 11 years ago

Closed 10 years ago

#7978 closed enhancement (fixed)

Cleanup of crystal code

Reported by: jbandlow Owned by: sage-combinat
Priority: major Milestone: sage-4.3.3
Component: combinatorics Keywords: crystals
Cc: sage-combinat Merged in: sage-4.3.3.alpha0
Authors: Nicolas M. Thiéry, Anne Schilling Reviewers: Dan Bump
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by nthiery)

Fixed some issues in crystals, such as

  • Comparison of elements
  • Latex output
  • Fixes whitespaces in /combinat/crystals/affine.py (as reported by Jason Bandlow)
  • All crystals have unique representation
  • Preparation of categorification of crystals:
    • C.element_class -> C.Element
    • All crystals are at least in the EnumeratedSets? category
    • Use rename, or define _repr_, instead of setting _name Eventually, crystals should only use _repr_ => Removed dependency upon deprecated CombinatorialClass?
  • Systematic use of TestSuite? instead of loads/dumps test
  • Fixed bug in fast_crystal: delpat is not immutable (which should be eventually be fixed), and was accidentally changed by the weight method). This was caught by turning on unique representation which made the crystals to be reused longer in the tests.

Depends on #8028 (element wrapper improvement)

Attachments (3)

trac_7978_fix_tabs_in_affine_py-jb.patch (31.7 KB) - added by jbandlow 11 years ago.
trac_7978_crystal_cleanup-as.patch (81.1 KB) - added by aschilling 11 years ago.
trac_7978_crystal_cleanup-as.2.patch (81.6 KB) - added by nthiery 11 years ago.
Updated patch updates one doctest.

Download all attachments as: .zip

Change History (11)

Changed 11 years ago by jbandlow

comment:1 Changed 11 years ago by jbandlow

  • Status changed from new to needs_work

Oops. This patch depends on some patches in the combinat stack. (I suspect the 'crystal-cleanup' patches.) The change is trivial, but I'll coordinate with the sage-combinat list before marking this ready for review.

Changed 11 years ago by aschilling

comment:2 Changed 11 years ago by aschilling

  • Authors set to Nicolas M. Thiery, Anne Schilling
  • Description modified (diff)
  • Keywords crystals added
  • Reviewers set to Dan Bump
  • Status changed from needs_work to needs_review
  • Summary changed from The file sage/combinat/affine.py contains lots of 'tab' characters to Cleanup of crystal code

The patch trac_7978_crystal_cleanup-as.patch supersedes Jason's patch. It fixes the whitespace issues in combinat/crystals/affine.py and does a lot more improvements in crystals (see description).

comment:3 Changed 11 years ago by nthiery

  • Authors changed from Nicolas M. Thiery, Anne Schilling to Nicolas M. Thiéry, Anne Schilling
  • Description modified (diff)
  • Type changed from defect to enhancement

Changed 11 years ago by nthiery

Updated patch updates one doctest.

comment:4 Changed 11 years ago by bump

  • Status changed from needs_review to positive_review

I checked that the patch passes sage --testall, either with or without #8028.

I checked that it does not break latex method for classical crystals or metapost method for fastcrystals. I think the latex changes are specific to affine crystals.

The patch does not seem to break anything, and fixes some problems. I recommend merging it.

comment:5 Changed 10 years ago by mpatel

I've added the ticket number in the refreshed patch I'm applying to 4.3.3.alpha0.

comment:6 Changed 10 years ago by bump

I'm having trouble parsing mpatel's message. Does this mean the patch is to be merged in 4.3.3.alpha0?

What "refreshed patch" does this message refer to? The patch trac_7978_crystal_cleanup-as.2.patch applies cleanly to sage-4.3.2 and does not break anything in sage/combinat/crystals.

comment:7 Changed 10 years ago by mpatel

I'll merge this ticket into 4.3.3.alpha0. I mean only that I've prepended the ticket number to the first line of the commit string of the patch I've added to my working tree for 4.3.3.alpha0. I apologize for being so cryptic.

comment:8 Changed 10 years ago by mpatel

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