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 )
Extending the alcove path model implementation to affine type.
Apply:
Attachments (3)
Change History (28)
comment:1 Changed 8 years ago by
- Description modified (diff)
Changed 8 years ago by
comment:2 Changed 8 years ago by
- Cc sage-combinat tscrim aschilling added
- Status changed from new to needs_review
Changed 8 years ago by
comment:3 follow-up: ↓ 4 Changed 8 years ago by
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
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: ↓ 6 Changed 8 years ago by
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
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: ↓ 8 Changed 8 years ago by
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: ↓ 9 Changed 8 years ago by
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
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: ↓ 12 Changed 8 years ago by
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
- Description modified (diff)
- Reviewers changed from Anne Schilling, to Anne Schilling
comment:12 in reply to: ↑ 10 ; follow-up: ↓ 14 Changed 8 years ago by
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
- 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
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
Hi Anne,
Review patch is folded, new patch is uploaded.
Arthur
comment:16 follow-up: ↓ 17 Changed 8 years ago by
- Status changed from needs_review to positive_review
comment:17 in reply to: ↑ 16 Changed 8 years ago by
Congratulations, Dr. Lubovsky (in two meanings!).
Anne
comment:18 Changed 8 years ago by
- Milestone changed from sage-5.10 to sage-5.11
comment:19 Changed 8 years ago by
- 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
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: ↓ 22 Changed 8 years ago by
Sorry about the errors, should be fixed now.
comment:22 in reply to: ↑ 21 Changed 8 years ago by
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
comment:23 Changed 8 years ago by
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
- Status changed from needs_work to positive_review
comment:25 Changed 8 years ago by
- Merged in set to sage-5.11.beta2
- Resolution set to fixed
- Status changed from positive_review to closed
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.