Implement a catalog for crystals #15882

tscrim · 2014-03-01T18:09:27Z

Make a catalog for crystals similar to graphs/codes/etc. and remove things like CrystalOf* and InfinityCrystalOf* from the global namespace in an effort to clean it up.

Depends on #14275
Depends on #16027

CC: @sagetrac-sage-combinat @nthiery @anneschilling @bsalisbury1

Component: combinatorics

Keywords: crystals catalog

Author: Travis Scrimshaw

Branch: 0942e81

Reviewer: Nathann Cohen, Anne Schilling

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

The text was updated successfully, but these errors were encountered:

tscrim · 2014-03-01T18:51:29Z

Commit: 56a29b8

tscrim · 2014-03-01T18:51:29Z

Branch: public/combinat/crystals/catalog

tscrim · 2014-03-01T18:51:29Z

comment:1

Starting to get there.

New commits:

`3babe7f`	`Initial setup.`
`629bb54`	`Merge branch 'develop' into public/combinat/crystals/catalog`
`b353b82`	`Merge branch 'develop' into public/combinat/crystals/catalog`
`acb0610`	`Merge branch 'develop' into public/combinat/crystals/catalog`
`fa15648`	`Merge branch 'develop' into public/combinat/crystals/catalog`
`7b79a42`	`Renamed catalog files.`
`56a29b8`	`Starting to deprecate from global namespace.`

nthiery · 2014-03-03T09:38:46Z

comment:2

Thanks Travis! I haven't checked the code yet, but this will be a nice improvement.

One thing I have been pondering for some time: at the end of the day, we can expect that there will eventually be exactly two entry points for crystals in the global name space, namely crystals and Crystals (indeed, once #10963 will be in, we will be able to deprecate FiniteCrystals() and friends in favor of Crystals().Finite()).

Would it make sense to push the logic even further and actually have a single entry point, which probably should be Crystals? In other words, could the category also play the role of catalog of implementations? Maybe something like Crystals.catalog.CrystalOfLetters(...) though this is a bit heavyweight.

Of course this question is not just about crystals, but could apply to groups, posets, ...

Anyway, just food for thought for after this ticket.

tscrim · 2014-03-03T16:08:18Z

comment:3

How about we link them all to the category? For example, currently I do

sage: crystals.Tableaux(['A',2], shape=[2,1])

Instead we make this use the category:

sage: Crystals.Tableaux(['A',2], shape=[2,1])

We should be able to do

from sage.combinat.crystals.catalog import *

in the crystals category to do this. Although doing things this way, we could easily have things lost among the category methods. So I'm not convinced this is the best direction to go.

Also I'm somewhat of the opinion that categories should be grouped together as well via categories.Crystals and such and not be in the global namespace. Therefore leaving crystals.* as the only entry point. IDK, I'm okay with 2 entry points too...

nthiery · 2014-03-03T17:04:38Z

comment:4

Replying to @tscrim:

How about we link them all to the category? For example, currently I do
sage: crystals.Tableaux(['A',2], shape=[2,1])
Instead we make this use the category:
sage: Crystals.Tableaux(['A',2], shape=[2,1])
We should be able to do
from sage.combinat.crystals.catalog import *
in the crystals category to do this. Although doing things this way,
we could easily have things lost among the category methods. So I'm
not convinced this is the best direction to go.

Agreed! This is why, in my attempted notation, I had put this in an
attribute Crystals.catalog. But it's not super convincing
either.

Also I'm somewhat of the opinion that categories should be grouped
together as well via categories.Crystals and such and not be in
the global namespace. Therefore leaving crystals.* as the only
entry point.

I agree that there definitely should be far less categories in the
global name space, and #10963 will help much in this
direction. However, for the well known ones like Sets, Fields, Groups,
Posets, Crystals, ... I find that the category is a natural entry
point to explore the functionalities implemented in Sage about a given
topic, because it has more semantic attached to it than with a mere
module like groups.

On a related matter, it would be a cool feature, when given a
category, to recover all parents in this category. E.g.:

    sage: affine_crystals = Crystals().Affine().implementations()
    sage: affine_crystals.K<tab>
    KirillovReshetikhinCrystal
    ...

Florent had implemented a proof-of-concept, recovering automatically
the information from a database build from the Sage TestSuite's. But
this would take some more work.

IDK, I'm okay with 2 entry points too...

That will certainly do for now :-)

Cheers,
Nicolas

sagetrac-git · 2014-03-17T15:06:11Z

Changed commit from 56a29b8 to a27f7fa

sagetrac-git · 2014-03-17T15:06:11Z

Branch pushed to git repo; I updated commit sha1. New commits:

`6dc2259`	`Deprecated CrystalOfSpins* and KyotoPathModel.`
`441a478`	`Merge branch 'develop' into public/combinat/crystals/catalog`
`ed62d74`	`Added deprecations for affine.py.`
`108e019`	`Adds a deprecation option to lazy_import.`
`a484809`	`Doctest fixes.`
`7c3558a`	`Merge branch 'u/tscrim/14275' into public/combinat/crystals/catalog-15882`
`fbe3f6c`	`Added all remaining deprecations.`
`a27f7fa`	`Merge branch 'develop' into public/combinat/crystals/catalog-15882`

tscrim · 2014-03-17T15:08:00Z

Dependencies: #14275

tscrim · 2014-03-17T15:08:00Z

comment:6

Using #14275 makes deprecating things from the global namespace so much easier.

sagetrac-git · 2014-03-19T15:58:06Z

Branch pushed to git repo; I updated commit sha1. New commits:

`ee4789f`	`Easier access to KR crystals and converted elementary_crystals.py and infinity_crystals.py.`
`a695b22`	`Finished replacing deprecated public tests and did some pyflakes cleanup.`
`2167fed`	`Fixing documentation for highest_weight_crystals.py.`

sagetrac-git · 2014-03-19T15:58:06Z

Changed commit from a27f7fa to 2167fed

sagetrac-git · 2014-03-19T17:01:58Z

Branch pushed to git repo; I updated commit sha1. New commits:

`fa08cf3`	`Used catalog in thematic tutorials and some additional cleanup/updating.`
`420cd33`	Fixed linking of :class:`...` in doc.

sagetrac-git · 2014-03-19T17:01:58Z

Changed commit from 2167fed to 420cd33

tscrim · 2014-03-19T17:02:53Z

comment:10

Okay, now it should be ready.

nathanncohen · 2014-03-20T13:51:28Z

comment:11

(being curious. I love catalogs)

nathanncohen · 2014-03-20T14:52:42Z

comment:12

Hey man.

I felt a bit trap when I noticed, after having spent quite some time reading the diff, that the ticket did not only change what it claims....

Anyway.

I agree with everything I can understand (and "git diff --word-diff" really is useful for tickets like that), but there are five things that I cannot review:

The "model" changes to /src/sage/combinat/crystals/highest_weight_crystals.py
This change

-        if self.cartan_type().is_affine():
+        if not self.cartan_type().is_finite():

This one

-        return other.__lt__(self)
+        return self._list.__gt__(other._list)

The definition of KirillovReshetikhin in catalog.py (and I don't think that it is the right place to define it)
Same for RiggedConfigurations in catalog_kirillov_reshetikhin.py though the code is trivial>

I do not understand what it does, and though I thought I could review a ticket that implements a crystal catalogs, I can't review these parts.

Soooo well. You can either remove them and I can set this ticket to positive review, or someone else will do it.

There are broken doc links reported by sage -docbuild reference/combinat --warn-links html. And I also add a commit to your branch to fix a typo and a "double deprecation".

Nathann

sagetrac-git · 2014-03-20T14:53:15Z

Branch pushed to git repo; I updated commit sha1. New commits:

`78489a6`	`trac #15882: review (typos)`

sagetrac-git · 2014-03-20T14:53:15Z

Changed commit from 420cd33 to 78489a6

nathanncohen · 2014-03-20T14:57:44Z

comment:14

Oh and I forgot something important : the new index of crystals must appear in the reference manual !

Nathann

tscrim · 2014-03-21T02:28:23Z

comment:15

Hey Nathann,

Replying to @nathanncohen:

I felt a bit trap when I noticed, after having spent quite some time reading the diff, that the ticket did not only change what it claims....

Well, there were things that I wanted to touch up while I was making sweeping changes.

I agree with everything I can understand (and "git diff --word-diff" really is useful for tickets like that), but there are five things that I cannot review:

The "model" changes to /src/sage/combinat/crystals/highest_weight_crystals.py

This change
-        if self.cartan_type().is_affine():
+        if not self.cartan_type().is_finite():

For reference: this is for better (future?) support for LS paths over other indefinite types (ex. hyperbolic).

This one

-        return other.__lt__(self)
+        return self._list.__gt__(other._list)

After some though, I felt it was better to revert this.

The definition of KirillovReshetikhin in catalog.py (and I don't think that it is the right place to define it)

This is the right place because it's just a function which redirects to the correct class (in different files). IMO it's stupid (and potentially confusing) to put it in kirillov_reshetikhin.py just to import it into the catalog.

Same for RiggedConfigurations in catalog_kirillov_reshetikhin.py though the code is trivial>

Here this should be here even more so because I want to reinforce that we only want to construct the rigged configurations of a single KR crystal (not as a tensor product thereof).

I do not understand what it does, and though I thought I could review a ticket that implements a crystal catalogs, I can't review these parts.

Thank you for your review thus far.

There are broken doc links reported by sage -docbuild reference/combinat --warn-links html. And I also add a commit to your branch to fix a typo and a "double deprecation".

I've done what I can about them.

Best,

Travis

sagetrac-git · 2014-03-21T02:28:39Z

Changed commit from 78489a6 to 968f9f6

sagetrac-git · 2014-04-11T21:47:10Z

Branch pushed to git repo; I updated commit sha1. New commits:

`6402c54`	`fixed some documentation issues`

sagetrac-git · 2014-04-12T14:45:12Z

Changed commit from 6402c54 to a596cdd

sagetrac-git · 2014-04-12T14:45:12Z

Branch pushed to git repo; I updated commit sha1. New commits:

`482ca9c`	`Fixed last bad link.`
`a596cdd`	`Merge branch 'public/combinat/crystals/catalog' of trac.sagemath.org:sage into public/combinat/crystals/catalog-15882`

tscrim · 2014-04-12T14:50:32Z

comment:45

Well, that was the only bad link I could find with an extensive grep campaign.

sagetrac-git · 2014-04-13T03:33:00Z

Branch pushed to git repo; I updated commit sha1. New commits:

`698ae7e`	`some small changes to the documentation`

sagetrac-git · 2014-04-13T03:33:00Z

Changed commit from a596cdd to 698ae7e

anneschilling · 2014-04-13T03:34:12Z

comment:47

Hi Travis,

I just pushed some small changes to the introduction. If you agree with them, please set a positive review on my behalf!

Anne

tscrim · 2014-04-13T03:41:01Z

comment:48

Looks good to me. Thanks Anne and Nathann for doing the review!

vbraun · 2014-04-13T21:03:15Z

comment:49

sage -t --long src/sage/combinat/root_system/plot.py  # 1 doctest failed
sage -t --long src/sage/combinat/tableau.py  # 1 doctest failed
sage -t --long src/sage/combinat/root_system/cartan_type.py  # 2 doctests failed

sagetrac-git · 2014-04-14T14:58:59Z

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

`0942e81`	`Fixed failing doctests.`

sagetrac-git · 2014-04-14T14:58:59Z

Changed commit from 698ae7e to 0942e81

tscrim · 2014-04-14T14:59:32Z

comment:51

Sorry about that.

vbraun · 2014-04-14T19:57:22Z

Changed branch from public/combinat/crystals/catalog to 0942e81

nthiery · 2014-05-09T09:23:50Z

comment:53

Hi!

Sorry, I haven't been been following up on this ticket, so that's probably too late. Anyway, just as a food for thought, I wanted to mention that I was surprised at first to have

    crystals.Letters

rather than

    crystals.CrystalOfLetters

The latter is nicer if you want to do from crystals import *. But maybe the point is that you don't want to do that and just always crystals...., and then it's less redundant.

Let's see how this is done in the other catalogs. Hmm, we have graphs.ChvatalGraph, but groups.permutation.Dihedral. Not super consistent ...

Cheers,
Nicolas

nthiery · 2014-05-09T09:23:50Z

Changed commit from 0942e81 to none

anneschilling · 2014-05-09T14:08:29Z

comment:54

Hi Nicolas,

We thought that

crystals.CrystalOfLetters

was redundant. Since it is pretty short, I think I/we will just use
crystals. ... in the future.

Anne

nathanncohen · 2014-05-12T08:42:48Z

comment:55

Let's see how this is done in the other catalogs. Hmm, we have graphs.ChvatalGraph, but groups.permutation.Dihedral. Not super consistent ...

As always, it is better to make sense than to be consistent. The Graph thing is an error. We should .... deprecated those 100+ functions and remove the final "Graph" ?... Scary T_T

Nathann

tscrim added this to the sage-6.2 milestone Mar 1, 2014

tscrim added c: combinatorics labels Mar 1, 2014

tscrim assigned sagetrac-sage-combinat Mar 1, 2014

tscrim added the s: needs review label Mar 19, 2014

nathanncohen mannequin added s: needs info and removed s: needs review labels Mar 20, 2014

tscrim added s: positive review and removed s: needs review labels Apr 13, 2014

sagetrac-git mannequin added s: needs review and removed s: positive review labels Apr 14, 2014

tscrim added s: positive review and removed s: needs review labels Apr 14, 2014

vbraun removed the s: positive review label Apr 14, 2014

vbraun closed this as completed in 0a5f3eb Apr 14, 2014

This was referenced Apr 13, 2014

Implement crystal morphisms, subcrystals, and virtual crystals #15463

Closed

Add methods to rigged configurations #15560

Closed

Make the tensor functorial construction work for crystals #16001

Closed

zabrocki mannequin mentioned this issue Nov 26, 2015

remove expired deprecations in sage/combinat #19513

Closed

fchapoton mentioned this issue Sep 13, 2018

remove most deprecated stuff in crystals #26247

Closed

Implement a catalog for crystals #15882

Implement a catalog for crystals #15882

Comments

tscrim commented Mar 1, 2014

tscrim commented Mar 1, 2014

tscrim commented Mar 1, 2014

tscrim commented Mar 1, 2014

nthiery commented Mar 3, 2014

tscrim commented Mar 3, 2014

nthiery commented Mar 3, 2014

sagetrac-git mannequin commented Mar 17, 2014

sagetrac-git mannequin commented Mar 17, 2014

tscrim commented Mar 17, 2014

tscrim commented Mar 17, 2014

sagetrac-git mannequin commented Mar 19, 2014

sagetrac-git mannequin commented Mar 19, 2014

sagetrac-git mannequin commented Mar 19, 2014

sagetrac-git mannequin commented Mar 19, 2014

tscrim commented Mar 19, 2014

nathanncohen mannequin commented Mar 20, 2014

nathanncohen mannequin commented Mar 20, 2014

sagetrac-git mannequin commented Mar 20, 2014

sagetrac-git mannequin commented Mar 20, 2014

nathanncohen mannequin commented Mar 20, 2014

tscrim commented Mar 21, 2014

sagetrac-git mannequin commented Mar 21, 2014

sagetrac-git mannequin commented Apr 11, 2014

sagetrac-git mannequin commented Apr 12, 2014

sagetrac-git mannequin commented Apr 12, 2014

tscrim commented Apr 12, 2014

sagetrac-git mannequin commented Apr 13, 2014

sagetrac-git mannequin commented Apr 13, 2014

anneschilling commented Apr 13, 2014

tscrim commented Apr 13, 2014

vbraun commented Apr 13, 2014

sagetrac-git mannequin commented Apr 14, 2014

sagetrac-git mannequin commented Apr 14, 2014

tscrim commented Apr 14, 2014

vbraun commented Apr 14, 2014

nthiery commented May 9, 2014

nthiery commented May 9, 2014

anneschilling commented May 9, 2014

nathanncohen mannequin commented May 12, 2014