-
-
Notifications
You must be signed in to change notification settings - Fork 481
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Alcove model in affine cartan type #14143
Comments
comment:1
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. |
This comment has been minimized.
This comment has been minimized.
comment:2
Attachment: trac_14143-alcove-path-al.patch.gz |
Attachment: trac_14143-alcove-path-al.2.patch.gz |
comment:3
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, |
comment:4
Hi Arthur, Thanks for your patch on alcove paths. Here are some further comments that would be good to incorporate!
So much for now! Anne |
comment:5
Hi Anne,
Arthur |
comment:6
Hi Arthur, Thanks for your work on this!
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!
Look at
Ok, then please make them underscore methods!
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
Hi Anne,
thanks |
comment:8
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
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
Is the following really the correct behavior? Do you want alcove paths crystal labeled by level zero weights to be highest weight crystals?
Best, Anne |
comment:10
Hi Anne,
me. I'm using the hgrc from
Arthur |
This comment has been minimized.
This comment has been minimized.
Changed reviewer from Anne Schilling, to Anne Schilling |
comment:12
Hi Arthur, Thanks for making the changes!
The header looks ok to me now. I usually do
and then edit the header to put the line #14143. Then you need to do
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
|
comment:14
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
Hi Anne,
Arthur |
comment:17
Congratulations, Dr. Lubovsky (in two meanings!). Anne |
comment:19
Never use a bare
Always catch specific exceptions, or, if you really want a catch-all, use
|
comment:20
There are problems with the documentation:
I also recommend you to use the new doctest continuations (
|
comment:21
Sorry about the errors, should be fixed now. |
comment:22
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 |
Attachment: trac_14143-alcove-path-al.3.patch.gz |
comment:23
Hi Anne,
Arthur |
Merged: sage-5.11.beta2 |
Extending the alcove path model implementation to affine type.
Apply:
CC: @sagetrac-sage-combinat @tscrim @anneschilling
Component: combinatorics
Keywords: alcove model, days45
Author: Arthur Lubovsky
Reviewer: Anne Schilling
Merged: sage-5.11.beta2
Issue created by migration from https://trac.sagemath.org/ticket/14143
The text was updated successfully, but these errors were encountered: