Opened 13 years ago

Closed 13 years ago

#7978 closed enhancement (fixed)

Cleanup of crystal code

Reported by: Jason Bandlow Owned by: Sage Combinat CC user
Priority: major Milestone: sage-4.3.3
Component: combinatorics Keywords: crystals
Cc: Sage Combinat CC user 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:

Status badges

Description (last modified by Nicolas M. Thiéry)

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 Jason Bandlow 13 years ago.
trac_7978_crystal_cleanup-as.patch (81.1 KB) - added by Anne Schilling 13 years ago.
trac_7978_crystal_cleanup-as.2.patch (81.6 KB) - added by Nicolas M. Thiéry 13 years ago.
Updated patch updates one doctest.

Download all attachments as: .zip

Change History (11)

Changed 13 years ago by Jason Bandlow

comment:1 Changed 13 years ago by Jason Bandlow

Status: newneeds_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 13 years ago by Anne Schilling

comment:2 Changed 13 years ago by Anne Schilling

Authors: Nicolas M. Thiery, Anne Schilling
Description: modified (diff)
Keywords: crystals added
Reviewers: Dan Bump
Status: needs_workneeds_review
Summary: The file sage/combinat/affine.py contains lots of 'tab' charactersCleanup 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 13 years ago by Nicolas M. Thiéry

Authors: Nicolas M. Thiery, Anne SchillingNicolas M. Thiéry, Anne Schilling
Description: modified (diff)
Type: defectenhancement

Changed 13 years ago by Nicolas M. Thiéry

Updated patch updates one doctest.

comment:4 Changed 13 years ago by Daniel Bump

Status: needs_reviewpositive_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 13 years ago by Mitesh Patel

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

comment:6 Changed 13 years ago by Daniel 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 13 years ago by Mitesh Patel

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 13 years ago by Mitesh Patel

Merged in: sage-4.3.3.alpha0
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.