Py3: Fix crystals.kirillov_reshetikhin for python3

[vinklein]

Fix crystals.kirillov_reshetikhin doctests for python3

CC: @tscrim

Component: python3

Author: Vincent Klein

Branch/Commit: dc9330d

Reviewer: Travis Scrimshaw

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

[vinklein]

Branch: u/vklein/27977

[sagetrac-git]

Commit: d3971c3

[sagetrac-git]

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

d3971c3

Trac #27977: use OrderedDict in from_A7_crystal

[vinklein]

comment:5

Note that from_A7_crystal call sorted two times.

If there is high performances expectations with these function, it might be best to juste tag the doctests with #py2 / #py3.

[tscrim]

comment:7

IMO, it is bad form to change (non print output) code so doctests are sorted. Instead you should just sorted(K._highest_weight_to_A7_elements.items()) and do the following change to CrystalMorphismByGenerators (in categories/crystals.py):

         if gens is None:
             if isinstance(on_gens, dict):
-                gens = on_gens.keys()
+                gens = sorted(on_gens)
             else:
                 gens = parent.domain().module_generators
         self._gens = tuple(gens)

[tscrim]

Reviewer: Travis Scrimshaw

[vinklein]

comment:9

Ok thanks for your comments.

Your solution doesn't solve the case with from_A7_crystal, it looks likes one difference is that gens is not None when CrystalMorphismByGenreators.__init__ is called. I am currently searching why this difference.

It's not clear to me why sorting at CrystalMorphismByGenerators level is cleaner than doing it at KR_type_E7 level. Can you expand on that please ?

My initial reasoning was that as it is a KR_type_E7 specific problem the fix should not impact other classes.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

b6c5518

Trac #27977:Fix crystals.kirillov_reshetikhin ...

[sagetrac-git]

Changed commit from d3971c3 to b6c5518

[vinklein]

comment:11

Here is another proposal with fixes at CrystalMorphismByGenerators level.

[vinklein]

comment:13

Sorry for the noise

[tscrim]

comment:14

Replying to @vinklein:

Ok thanks for your comments.

Your solution doesn't solve the case with from_A7_crystal, it looks likes one difference is that gens is not None when CrystalMorphismByGenreators.__init__ is called. I am currently searching why this difference.

Ah, right. I misread it as gens is not None. ^^;; Actually, it really is just an output issue. This has zero impact on the code working (at least, it should otherwise I would suspect a bug).

It's not clear to me why sorting at CrystalMorphismByGenerators level is cleaner than doing it at KR_type_E7 level. Can you expand on that please ?

My initial reasoning was that as it is a KR_type_E7 specific problem the fix should not impact other classes.

It is not specific to that. It just happens to be that this case constructs the morphism using a (complicated enough) dict and this is not done elsewhere in any doctest. So this would cause an output difference in those doctests.

I am still not 100% happy with the change I proposed as it is essentially an unnecessary sort. However, I understand why you would do this for compatibility, and I doubt anyone is doing something with this where that would become a performance bottleneck. Well, this will now work (although we could force the sorting, but that would likely trivially break a bunch of other doctests):

         if gens is None:
             if isinstance(on_gens, dict):
-                gens = on_gens.keys()
+                gens = on_gens
             else:
                 gens = parent.domain().module_generators
+        if isinstance(gens, dict):
+            gens = sorted(gens)
         self._gens = tuple(gens)

I am definitely in favor of just changing the doctest to have ... for the images of the generators for the morphism.

---

New commits:

b6c5518

Trac #27977:Fix crystals.kirillov_reshetikhin ...

[tscrim]

comment:15

You just beat me to it. I still think it would be good to remove that useless call to keys() in the if gens is None: block, but if you don't want to do that, then you can set a positive review.

[vinklein]

comment:16

Well the current branch doesn't have side effects on others doctests under combinat/crystals from the tests i have done.

To be accurate you're fine with this solution (minus the keys() call) but you prefer to just use ... in doctests ?

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

dc9330d

Trac #27977:Fix crystals.kirillov_reshetikhin ...

[sagetrac-git]

Changed commit from b6c5518 to dc9330d

[vinklein]

comment:18

I think you are right using ... may be the cleaner solution here.

Third branch has been pushed.

[tscrim]

comment:19

Thank you. The doctest is really just about showing the morphism exists, the actual definition is tested through other means.

[vbraun]

Changed branch from u/vklein/27977 to dc9330d

[embray]

comment:21

Not in Sage 8.8. Let's please to try keep tickets' milestones related to the release in which we actually intend to include them, and in particular the release in which they were actually included, especially when closing tickets.