Opened 5 months ago

Closed 5 months ago

Last modified 4 months ago

#27977 closed enhancement (fixed)

Py3: Fix crystals.kirillov_reshetikhin for python3

Reported by: vklein Owned by:
Priority: major Milestone: sage-8.9
Component: python3 Keywords:
Cc: tscrim Merged in:
Authors: Vincent Klein Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: dc9330d (Commits) Commit: dc9330d8e6001e2b817651e2459a897955153e32
Dependencies: Stopgaps:

Description (last modified by vklein)

Fix crystals.kirillov_reshetikhin doctests for python3

Change History (21)

comment:1 Changed 5 months ago by vklein

  • Description modified (diff)

comment:2 Changed 5 months ago by vklein

  • Branch set to u/vklein/27977

comment:3 Changed 5 months ago by git

  • Commit set to d3971c34cf2558901e9d6ccd17286d7e538e1e43

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

d3971c3Trac #27977: use OrderedDict in from_A7_crystal

comment:4 Changed 5 months ago by vklein

  • Status changed from new to needs_review

comment:5 Changed 5 months ago by vklein

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 5 months ago by chapoton

  • Cc tscrim added

comment:7 Changed 5 months ago by tscrim

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 5 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to needs_work

comment:9 follow-up: Changed 5 months ago by vklein

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.

Last edited 5 months ago by vklein (previous) (diff)

comment:10 Changed 5 months ago by git

  • Commit changed from d3971c34cf2558901e9d6ccd17286d7e538e1e43 to b6c5518f7f9daa0a69aaba2bcf14b6cb1b285036

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

b6c5518Trac #27977:Fix crystals.kirillov_reshetikhin ...

comment:11 Changed 5 months ago by vklein

Here is another proposal with fixes at CrystalMorphismByGenerators level.

comment:12 Changed 5 months ago by vklein

  • Description modified (diff)

comment:13 Changed 5 months ago by vklein

  • Status changed from needs_work to needs_review

Sorry for the noise

comment:14 in reply to: ↑ 9 Changed 5 months ago by tscrim

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 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:

b6c5518Trac #27977:Fix crystals.kirillov_reshetikhin ...

comment:15 Changed 5 months ago by tscrim

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 5 months ago by vklein

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 5 months ago by git

  • Commit changed from b6c5518f7f9daa0a69aaba2bcf14b6cb1b285036 to dc9330d8e6001e2b817651e2459a897955153e32

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

dc9330dTrac #27977:Fix crystals.kirillov_reshetikhin ...

comment:18 Changed 5 months ago by vklein

  • Description modified (diff)

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

Third branch has been pushed.

comment:19 Changed 5 months ago by tscrim

  • 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 5 months ago by vbraun

  • Branch changed from u/vklein/27977 to dc9330d8e6001e2b817651e2459a897955153e32
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:21 Changed 4 months ago by embray

  • Milestone changed from sage-8.8 to sage-8.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.

Note: See TracTickets for help on using tickets.