Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup of crystal code #7978

Closed
jbandlow opened this issue Jan 18, 2010 · 15 comments
Closed

Cleanup of crystal code #7978

jbandlow opened this issue Jan 18, 2010 · 15 comments

Comments

@jbandlow
Copy link

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)

CC: @sagetrac-sage-combinat

Component: combinatorics

Keywords: crystals

Author: Nicolas M. Thiéry, Anne Schilling

Reviewer: Dan Bump

Merged: sage-4.3.3.alpha0

Issue created by migration from https://trac.sagemath.org/ticket/7978

@jbandlow
Copy link
Author

comment:1

Attachment: trac_7978_fix_tabs_in_affine_py-jb.patch.gz

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.

@anneschilling
Copy link

Attachment: trac_7978_crystal_cleanup-as.patch.gz

@anneschilling

This comment has been minimized.

@anneschilling
Copy link

comment:2

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).

@anneschilling
Copy link

Changed keywords from none to crystals

@anneschilling
Copy link

Author: Nicolas M. Thiery, Anne Schilling

@anneschilling
Copy link

Reviewer: Dan Bump

@anneschilling anneschilling changed the title The file sage/combinat/affine.py contains lots of 'tab' characters Cleanup of crystal code Jan 25, 2010
@nthiery
Copy link
Contributor

nthiery commented Jan 25, 2010

Changed author from Nicolas M. Thiery, Anne Schilling to Nicolas M. Thiéry, Anne Schilling

@nthiery

This comment has been minimized.

@nthiery
Copy link
Contributor

nthiery commented Jan 27, 2010

Updated patch updates one doctest.

@dwbump
Copy link
Mannequin

dwbump mannequin commented Feb 2, 2010

comment:4

Attachment: trac_7978_crystal_cleanup-as.2.patch.gz

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.

@dwbump dwbump mannequin added s: positive review and removed s: needs review labels Feb 2, 2010
@qed777
Copy link
Mannequin

qed777 mannequin commented Feb 10, 2010

comment:5

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

@dwbump
Copy link
Mannequin

dwbump mannequin commented Feb 11, 2010

comment:6

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.

@qed777
Copy link
Mannequin

qed777 mannequin commented Feb 11, 2010

comment:7

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.

@qed777
Copy link
Mannequin

qed777 mannequin commented Feb 11, 2010

Merged: sage-4.3.3.alpha0

@qed777 qed777 mannequin removed the s: positive review label Feb 11, 2010
@qed777 qed777 mannequin closed this as completed Feb 11, 2010
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants