#27977 closed enhancement (fixed)
Py3: Fix crystals.kirillov_reshetikhin for python3
Reported by:  vklein  Owned by:  

Priority:  major  Milestone:  sage8.9 
Component:  python3  Keywords:  
Cc:  tscrim  Merged in:  
Authors:  Vincent Klein  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  dc9330d (Commits, GitHub, GitLab)  Commit:  dc9330d8e6001e2b817651e2459a897955153e32 
Dependencies:  Stopgaps: 
Description (last modified by )
Fix crystals.kirillov_reshetikhin doctests for python3
Change History (21)
comment:1 Changed 2 years ago by
 Description modified (diff)
comment:2 Changed 2 years ago by
 Branch set to u/vklein/27977
comment:3 Changed 2 years ago by
 Commit set to d3971c34cf2558901e9d6ccd17286d7e538e1e43
comment:4 Changed 2 years ago by
 Status changed from new to needs_review
comment:5 Changed 2 years ago by
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.
comment:6 Changed 2 years ago by
 Cc tscrim added
comment:7 Changed 2 years ago by
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)
comment:8 Changed 2 years ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to needs_work
comment:9 followup: ↓ 14 Changed 2 years ago by
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 KR_type_E7 specific problem the fix should not impact other classes.
comment:10 Changed 2 years ago by
 Commit changed from d3971c34cf2558901e9d6ccd17286d7e538e1e43 to b6c5518f7f9daa0a69aaba2bcf14b6cb1b285036
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
b6c5518  Trac #27977:Fix crystals.kirillov_reshetikhin ...

comment:11 Changed 2 years ago by
Here is another proposal with fixes at CrystalMorphismByGenerators
level.
comment:12 Changed 2 years ago by
 Description modified (diff)
comment:13 Changed 2 years ago by
 Status changed from needs_work to needs_review
Sorry for the noise
comment:14 in reply to: ↑ 9 Changed 2 years ago by
Replying to vklein:
Ok thanks for your comments.
Your solution doesn't solve the case with
from_A7_crystal
, it looks likes one difference is thatgens
is notNone
whenCrystalMorphismByGenreators.__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 atKR_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 ...

comment:15 Changed 2 years ago by
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.
comment:16 Changed 2 years ago by
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 ?
comment:17 Changed 2 years ago by
 Commit changed from b6c5518f7f9daa0a69aaba2bcf14b6cb1b285036 to dc9330d8e6001e2b817651e2459a897955153e32
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
dc9330d  Trac #27977:Fix crystals.kirillov_reshetikhin ...

comment:18 Changed 2 years ago by
 Description modified (diff)
I think you are right using ...
may be the cleaner solution here.
Third branch has been pushed.
comment:19 Changed 2 years ago by
 Status changed from needs_review to positive_review
Thank you. The doctest is really just about showing the morphism exists, the actual definition is tested through other means.
comment:20 Changed 2 years ago by
 Branch changed from u/vklein/27977 to dc9330d8e6001e2b817651e2459a897955153e32
 Resolution set to fixed
 Status changed from positive_review to closed
comment:21 Changed 2 years ago by
 Milestone changed from sage8.8 to sage8.9
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.
Branch pushed to git repo; I updated commit sha1. New commits:
Trac #27977: use OrderedDict in from_A7_crystal