Opened 8 years ago

Closed 8 years ago

#14143 closed enhancement (fixed)

Alcove model in affine cartan type

Reported by: alubovsky Owned by: sage-combinat
Priority: major Milestone: sage-5.11
Component: combinatorics Keywords: alcove model, days45
Cc: sage-combinat, tscrim, aschilling Merged in: sage-5.11.beta2
Authors: Arthur Lubovsky Reviewers: Anne Schilling
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by aschilling)

Extending the alcove path model implementation to affine type.

Apply:

Attachments (3)

trac_14143-alcove-path-al.patch (75.3 KB) - added by alubovsky 8 years ago.
trac_14143-alcove-path-al.2.patch (76.8 KB) - added by alubovsky 8 years ago.
trac_14143-alcove-path-al.3.patch (81.0 KB) - added by alubovsky 8 years ago.

Download all attachments as: .zip

Change History (28)

comment:1 Changed 8 years ago by alubovsky

  • Description modified (diff)

The second patch replaces the first, but there does not appear to be a way to do that if one forgets to click "replace patch with the same name" when submitting the patch initially.

Edit: The third patch is the latest one. It would be nice to be able to remove the first two.

Last edited 8 years ago by alubovsky (previous) (diff)

Changed 8 years ago by alubovsky

comment:2 Changed 8 years ago by alubovsky

  • Cc sage-combinat tscrim aschilling added
  • Status changed from new to needs_review

Changed 8 years ago by alubovsky

comment:3 follow-up: Changed 8 years ago by nthiery

Hi Arthur,

I just did a trivial rebase of your patch in the queue: it did not apply on all.py due to the addition of CrystalOfProjectedLevelZeroLSPaths yesterday.

By the way: there is no reason to import CrystalOfAlcovePathsElement? in the global name space. So I removed that. Also your patch removes the former ClassicalCrystalOfAlcovePaths?. This is good since the new version is more general; however a deprecation link should inserted.

See: http://www.sagemath.org/doc/developer/coding_in_python.html#deprecation

Cheers,

Nicolas

comment:4 in reply to: ↑ 3 Changed 8 years ago by aschilling

Hi Arthur,

Thanks for your patch on alcove paths. Here are some further comments that would be good to incorporate!

  • Please specify what the default value of good_zero_arrows is. Do you want to merge this experimental code into main sage, or rather separate it out into a personal patch that can be used by yourself and your collaborators (and me :-) )?
  • "Littlewood paths" in the documentation of sage.combinat.crystals.alcove_path.CrystalOfAlcovePaths? should be "Littelmann paths"
  • "We verify the alcove path crystal with the LS path crystal" should probably be "We verify that the alcove path crystal is isomorphic to the LS path crystal".
  • The interface for the LS paths has changed and now the preferred way is to input an actual weight rather than the coefficients of the fundamental weights. The advantage is that weights are typed whereas a list of coefficients is not. Do you think you could have a look at LS paths and make the user interface for Alcove paths consistent with it? I think this would be good for the users!
  • Since you mention Demazure 0-arrows in your documentation, please define what they are!
  • For the method vertices please explain how you compute all crystal elements without the crystal operators (since you say that they cannot all be reached by crystal operators; so you must have an alternative way!).
  • For the method :meth:digraph_fast perhaps give an example of how the speed compares to the usual digraph method and max_depth setting. You can do this with %timeit for example.
  • Why is the method static sign(root) in alcove paths and not in root_systems? The same question holds for max_level.
  • Typo in :meth:words: "finte Cartan types" -> "finite Cartan types"
  • Please explain the inputs of sage.combinat.crystals.alcove_path.compare_graphs(g1, g2, node1, node2).
  • I get some doctest failures:
sage -t alcove_path.py
**********************************************************************
File "alcove_path.py", line 637, in sage.combinat.crystals.alcove_path.CrystalOfAlcovePathsElement.epsilon
Failed example:
    [c.epsilon(1) for c in C]
Exception raised:
    Traceback (most recent call last):
      File "/Applications/sage-5.9/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 466, in _run
        self.execute(example, compiled, test.globs)
      File "/Applications/sage-5.9/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 825, in execute
        exec compiled in globs
      File "<doctest sage.combinat.crystals.alcove_path.CrystalOfAlcovePathsElement.epsilon[1]>", line 1, in <module>
        [c.epsilon(Integer(1)) for c in C]
      File "/Applications/sage-5.9/local/lib/python2.7/site-packages/sage/combinat/crystals/alcove_path.py", line 647, in epsilon
        return super(CrystalOfAlcovePathsElement, self).epsilon(i)
      File "/Applications/sage-5.9/local/lib/python2.7/site-packages/sage/misc/abstract_method.py", line 224, in __get__
        raise NotImplementedError(repr(self))
    NotImplementedError: <abstract method epsilon at 0x10a74ab90>
**********************************************************************
File "alcove_path.py", line 639, in sage.combinat.crystals.alcove_path.CrystalOfAlcovePathsElement.epsilon
Failed example:
    [c.epsilon(2) for c in C]
Exception raised:
    Traceback (most recent call last):
      File "/Applications/sage-5.9/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 466, in _run
        self.execute(example, compiled, test.globs)
      File "/Applications/sage-5.9/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 825, in execute
        exec compiled in globs
      File "<doctest sage.combinat.crystals.alcove_path.CrystalOfAlcovePathsElement.epsilon[2]>", line 1, in <module>
        [c.epsilon(Integer(2)) for c in C]
      File "/Applications/sage-5.9/local/lib/python2.7/site-packages/sage/combinat/crystals/alcove_path.py", line 647, in epsilon
        return super(CrystalOfAlcovePathsElement, self).epsilon(i)
      File "/Applications/sage-5.9/local/lib/python2.7/site-packages/sage/misc/abstract_method.py", line 224, in __get__
        raise NotImplementedError(repr(self))
    NotImplementedError: <abstract method epsilon at 0x10a74ab90>
**********************************************************************
1 item had failures:
   2 of   4 in sage.combinat.crystals.alcove_path.CrystalOfAlcovePathsElement.epsilon
    [212 tests, 2 failures, 97.16 s]
----------------------------------------------------------------------
sage -t alcove_path.py  # 2 doctests failed
----------------------------------------------------------------------
Total time for all tests: 97.4 seconds
    cpu time: 94.8 seconds
    cumulative wall time: 97.2 seconds

So much for now!

Anne

comment:5 follow-up: Changed 8 years ago by alubovsky

Hi Anne,

If it's not mentioned below, I addressed it in the new patch on combinat queue.

  • Do you want to merge this experimental code into main sage, or rather separate it out into a personal patch that can be used by yourself and your collaborators (and me :-) )?

A patch would probably be ideal. Would it then permanently live at the very bottom of the queue? Or just something available in private (which may be easier to implement).

  • The interface for the LS paths has changed and now the preferred way is to input an actual weight rather than the coefficients of the fundamental weights. The advantage is that weights are typed whereas a list of coefficients is not. Do you think you could have a look at LS paths and make the user interface for Alcove paths consistent with it? I think this would be good for the users!

Still need to do this part. It's a bit tricky and I don't have much time at the moment. When I find some time... I guess in order to stay compatible with the old ClassicalCrystalOfAlcovePaths? code I have to do the same implementation as the LS paths code, instead of just changing the parameters by hand.

  • Why is the method static sign(root) in alcove paths and not in root_systems? The same question holds for max_level.

root_systems already has a is_positive_root method, sign is just something convenient for my purposes. Should I rename it to _sign? max_level is something specific to the alcove model. Can be renamed to _max_level. Since that is part of a private class (doesn't get imported into sage) I don't see the point.

FYI, On line 284, TestSuite?(C).run() (in the affine case( is now taking around 150sec, which it didn't used to (I added a #long time). Not sure why this is happening.

Arthur

comment:6 in reply to: ↑ 5 Changed 8 years ago by aschilling

Hi Arthur,

Thanks for your work on this!

  • Do you want to merge this experimental code into main sage, or rather separate it out into a personal patch that can be used by yourself and your collaborators (and me :-) )?

A patch would probably be ideal. Would it then permanently live at the very bottom of the queue? Or just something available in private (which may be easier to implement).

Yes, it could be a patch somewhere in the sage-combinat queue (after the "Needs Review" section). Although, the queue will disappear soon anyway, but if you want to share the code with others, it might be best to have a patch for it!

  • The interface for the LS paths has changed and now the preferred way is to input an actual weight rather than the coefficients of the fundamental weights. The advantage is that weights are typed whereas a list of coefficients is not. Do you think you could have a look at LS paths and make the user interface for Alcove paths consistent with it? I think this would be good for the users!

Still need to do this part. It's a bit tricky and I don't have much time at the moment. When I find some time... I guess in order to stay compatible with the old ClassicalCrystalOfAlcovePaths? code I have to do the same implementation as the LS paths code, instead of just changing the parameters by hand.

Look at classcall_private line 121 in littelmann_path.py and init line 158. That allows both inputs and hence you would stay compatible with the old code!

  • Why is the method static sign(root) in alcove paths and not in root_systems? The same question holds for max_level.

root_systems already has a is_positive_root method, sign is just something convenient for my purposes. Should I rename it to _sign? max_level is something specific to the alcove model. Can be renamed to _max_level. Since that is part of a private class (doesn't get imported into sage) I don't see the point.

Ok, then please make them underscore methods!

FYI, On line 284, TestSuite?(C).run() (in the affine case( is now taking around 150sec, which it didn't used to (I added a #long time). Not sure why this is happening.

Yes, I noticed that for me the code also took a long time to run! Perhaps some profiling would be a good idea to see what is taking up so much time.

Best,

Anne

comment:7 follow-up: Changed 8 years ago by alubovsky

Hi Anne,

I've removed the 'good_zero_arrows' option, as requested. It may be easiest to put back in once sage moves to git, so I would rather do that.

The interface is changed to that of LS paths, using the 'preferred way'.

Requested methods are now private.

I've briefly ran a profiler on TestSuite?(C).run() (now line 277). It seems to be calling the usual processor intensive methods more than before. Perhaps it is searching deeper in this affine crystal? This may or may not be related, but doc testing for LS paths code is also taking quite a bit longer than before.

I've pushed the changes to combinat, but didn't make a patch. This ticket already has too many patches ( my fault ).

thanks Arthur

comment:8 in reply to: ↑ 7 ; follow-up: Changed 8 years ago by aschilling

Hi Arthur,

Thanks for your work on this! You can click on "Replace existing attachment of the same name" to avoid creating a new patch each time you upload changes. Putting the patch on trac for the final stages helps insofar as the patchbot runs tests.

Also, your patch on the sage-combinat queue needs a proper "export" head.

Best,

Anne

comment:9 in reply to: ↑ 8 Changed 8 years ago by aschilling

Hi Arthur, again,

There were some small glitches in the documentation which I fixed in a review patch on the sage-combinat queue. If you are happy with those, please fold it into your patch.

Also, you mentioned that CrystalOfAlcovePath? now has the same entries as LittelmannPath?. However, you did not change the documentation of the input to reflect this. Could you please do so? Also, you will need to add some examples such as

    sage: R = RootSystem(['A',2])
    sage: P = R.weight_lattice()
    sage: La = P.fundamental_weights()
    sage: CrystalOfAlcovePaths(La[1])
    Highest weight crystal of alcove paths of type ['A', 2] and weight Lambda[1]
    sage: C = CrystalOfAlcovePaths(La[1])
    sage: B = CrystalOfAlcovePaths(['A',2],[1,0])
    sage: B==C
    True

Is the following really the correct behavior? Do you want alcove paths crystal labeled by level zero weights to be highest weight crystals?

sage: R = RootSystem(['A',2,1])
sage: P = R.weight_lattice()
sage: La = P.fundamental_weights()
sage: C = CrystalOfAlcovePaths(La[1]-La[0])
sage: C
Highest weight crystal of alcove paths of type ['A', 2, 1] and weight -Lambda[0] + Lambda[1]

Best,

Anne

comment:10 follow-up: Changed 8 years ago by alubovsky

Hi Anne,

I mostly fixed the patch header. I'm not sure how to add the Node Id to the header (It should be automatic, but it's not working for me. I'm using the hgrc from http://wiki.sagemath.org/combinat/MercurialStepByStep )

Patch is updated on trac server.

You are correct. I only want dominant integral weights. It should now raise a ValueError?. Some examples of LS paths style inputs are added as requested. Please let me know if more are needed.

I also replaced the word level with height as was suggested by you some time ago, as level has another meaning, (as in LevelZeroLSpaths)

Arthur

comment:11 Changed 8 years ago by aschilling

  • Description modified (diff)
  • Reviewers changed from Anne Schilling, to Anne Schilling

comment:12 in reply to: ↑ 10 ; follow-up: Changed 8 years ago by aschilling

Hi Arthur,

Thanks for making the changes!

I mostly fixed the patch header. I'm not sure how to add the Node Id to the header (It should be automatic, but it's not working for me. I'm using the hgrc from http://wiki.sagemath.org/combinat/MercurialStepByStep )

The header looks ok to me now. I usually do

hg qrefresh -e

and then edit the header to put the line #14143. Then you need to do

hg export trac_14143.... /home/arthur/trac_14143...

where ... stands for the rest of the name and then upload to trac.

I will look at your changes more closely soon and will report back if I find anything else.

Greetings from Tel Aviv,

Anne

Patch is updated on trac server.

You are correct. I only want dominant integral weights. It should now raise a ValueError?. Some examples of LS paths style inputs are added as requested. Please let me know if more are needed.

I also replaced the word level with height as was suggested by you some time ago, as level has another meaning, (as in LevelZeroLSpaths)

Arthur

comment:13 Changed 8 years ago by aschilling

  • Summary changed from alcove model in affine cartan type to Alcove model in affine cartan type

comment:14 in reply to: ↑ 12 Changed 8 years ago by aschilling

Hi Arthur,

I left another review patch on the sage-combinat server. If you are happy with my changes, please fold that review patch and upload again. Then we are very close to a positive review!

Anne

comment:15 Changed 8 years ago by alubovsky

Hi Anne,

Review patch is folded, new patch is uploaded.

Arthur

comment:16 follow-up: Changed 8 years ago by aschilling

  • Status changed from needs_review to positive_review

comment:17 in reply to: ↑ 16 Changed 8 years ago by aschilling

Congratulations, Dr. Lubovsky (in two meanings!).

Anne

comment:18 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.10 to sage-5.11

comment:19 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Never use a bare

except:

Always catch specific exceptions, or, if you really want a catch-all, use

except StandardError:

comment:20 Changed 8 years ago by jdemeyer

There are problems with the documentation:

dochtml.log:[combinat ] /mazur/release/merger/sage-5.11.beta2/local/lib/python2.7/site-packages/sage/combinat/crystals/alcove_path.py:docstring of sage.combinat.crystals.alcove_path.compare_graphs:4: WARNING: Inline interpreted text or phrase reference start-string without end-string.
dochtml.log:[combinat ] /mazur/release/merger/sage-5.11.beta2/local/lib/python2.7/site-packages/sage/combinat/crystals/alcove_path.py:docstring of sage.combinat.crystals.alcove_path.compare_graphs:5: WARNING: Inline literal start-string without end-string.

I also recommend you to use the new doctest continuations (....: instead of ...), as follows:

sage: for e in g.edges():  # long time 
....:     if e[0].phi(0) == 1 and e[2] == 0:
....:         g.delete_edge(e)

comment:21 follow-up: Changed 8 years ago by alubovsky

Sorry about the errors, should be fixed now.

comment:22 in reply to: ↑ 21 Changed 8 years ago by aschilling

Hi Arthur,

Did you actually fold my review patch? Looking at the patch I still see some issues with the documentation that I had fixed in the review patch I had posted on the sage-combinat queue.

Anne

Changed 8 years ago by alubovsky

comment:23 Changed 8 years ago by alubovsky

Hi Anne,

You are correct. Even though I folded your patch, I mistakenly discarded the changes afterwards. My apologies! The latest version of the patch has your changes applied.

Arthur

comment:24 Changed 8 years ago by aschilling

  • Status changed from needs_work to positive_review

comment:25 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.11.beta2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.