Opened 8 years ago

Closed 6 years ago

Last modified 5 years ago

#15375 closed enhancement (fixed)

Extended Affine Weyl Groups SD40

Reported by: bump Owned by: bump
Priority: major Milestone: sage-6.8
Component: combinatorics Keywords: days54, coxeter, days64, days65
Cc: sage-combinat, aschilling, nthiery, mshimo, tscrim Merged in:
Authors: Daniel Bump, Dan Orr, Anne Schilling, Mark Shimozono, Nicolas M. Thiéry Reviewers: Dan Bump, Anne Schilling
Report Upstream: N/A Work issues:
Branch: f549344 (Commits, GitHub, GitLab) Commit:
Dependencies: #10963, #14102 Stopgaps:

Status badges

Description (last modified by bump)

The first version of this patch implementing Affine Weyl Groups was written during Sage Days 40. The original contributers were Daniel Bump, Dan Orr, Anne Schilling, Mark Shimozono, Nicolas Thiery. Subsequently Mark Shimozono added multiple realizations and twisted types.

Attachments (1)

trac_15375-extended_affine_weyl_groups_sd40.patch (135.2 KB) - added by bump 8 years ago.
Implements Extended Affine Iwahori Hecke Algebras

Download all attachments as: .zip

Change History (138)

comment:1 Changed 8 years ago by bump

  • Owner changed from (none) to bump
  • Type changed from PLEASE CHANGE to enhancement

Changed 8 years ago by bump

Implements Extended Affine Iwahori Hecke Algebras

comment:2 Changed 8 years ago by bump

  • Description modified (diff)

comment:3 Changed 8 years ago by bump

  • Cc aschilling nthiery mshimo added

comment:4 Changed 8 years ago by bump

  • Description modified (diff)
  • Summary changed from Extended Affine Iwahori Hecke Algebras SD40 to Extended Affine Weyl Groups SD40

comment:5 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.0 to sage-6.1

comment:6 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:7 Changed 8 years ago by aschilling

  • Branch set to public/combinat/extended_affine_weyl_groups-15375
  • Commit set to 6d9dd29abed89e4b0217e644ba34f16902efbf4f

New commits:

0251a33Trac #13394: Implement faster and safer WeakValueDictionary
9d9cae3# Sat Oct 19 11:50:04 2013 +0200
b2914f3# Sun Oct 27 13:58:49 2013 +0100
362fd5e# Tue Oct 29 20:14:19 2013 +0100
dfc1fe8#14102: Nonsymmetric Macdonald polynomials
4c44e62imported patch extended_affine_weyl_groups_sd40.patch
6d9dd29fixed extended affine Weyl groups

comment:8 Changed 8 years ago by git

  • Commit changed from 6d9dd29abed89e4b0217e644ba34f16902efbf4f to 6e725643031d4780bad4483db626bca7ae553e01

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

6e72564added prefix support for extended affine Weyl groups

comment:9 Changed 8 years ago by tscrim

  • Cc tscrim added

comment:10 Changed 8 years ago by git

  • Commit changed from 6e725643031d4780bad4483db626bca7ae553e01 to e293db988bcda8694701c35d7dff4b6e5dfc525f

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

e293db9added dual form of translation lattices

comment:11 Changed 8 years ago by mshimo

Doctests now pass

comment:12 Changed 8 years ago by git

  • Commit changed from e293db988bcda8694701c35d7dff4b6e5dfc525f to 69b91167e2f82170992fa9c91b21105f4a33f7d5

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

69b9116fixed bug in type BCdual

comment:13 Changed 8 years ago by git

  • Commit changed from 69b91167e2f82170992fa9c91b21105f4a33f7d5 to 90ed1b11dc51dfcb454feff3dfbe95fe56b12b75

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

90ed1b1changed calling syntax

comment:14 Changed 8 years ago by chapoton

  • Keywords coxeter added

comment:15 Changed 8 years ago by git

  • Commit changed from 90ed1b11dc51dfcb454feff3dfbe95fe56b12b75 to fb8a6e096d087e8b0c7dd91e81717b9c33bde242

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

fb8a6e0fund gp special nodes returns tuple

comment:16 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:17 Changed 7 years ago by git

  • Commit changed from fb8a6e096d087e8b0c7dd91e81717b9c33bde242 to d534a0d3b583dc905ad02d962fb64d58cf5defb4

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

d534a0dextended affine weyl group for GL

comment:18 Changed 7 years ago by git

  • Commit changed from d534a0d3b583dc905ad02d962fb64d58cf5defb4 to dc20ae933c3354b2279446bd8df0d00283f00e80

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

dc20ae9fixed looking for memory leak

comment:19 Changed 7 years ago by git

  • Commit changed from dc20ae933c3354b2279446bd8df0d00283f00e80 to 7c1ed09b81c0e3cd4e898ba0941a898fcc36f0b3

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

7c1ed09initial merge of extended affine weyl group with gl

comment:20 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:21 Changed 7 years ago by git

  • Commit changed from 7c1ed09b81c0e3cd4e898ba0941a898fcc36f0b3 to db6924af6c99f3bfa679be505f6b1ca5d73d890d

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

f3cce2fmerge
db6924amore merging

comment:22 Changed 7 years ago by aschilling

  • Keywords days64 added
  • Status changed from new to needs_review

comment:23 Changed 7 years ago by git

  • Commit changed from db6924af6c99f3bfa679be505f6b1ca5d73d890d to 6708533d8357f38f656c758c53af6c7fb410def7

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

6708533removed FundamentalGroup etc from global name space

comment:24 Changed 7 years ago by git

  • Commit changed from 6708533d8357f38f656c758c53af6c7fb410def7 to 66e07a2bb515487a80d8ad494c4ef286d8a181cc

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

66e07a2changes to documentation

comment:25 Changed 7 years ago by bump

  • Reviewers set to bump

comment:26 Changed 7 years ago by bump

  • Description modified (diff)

comment:27 Changed 7 years ago by bump

All tests in combinat/root_system pass.

comment:28 Changed 7 years ago by bump

  • Status changed from needs_review to needs_work

We should add a line

sage/combinat/root_system/extended_affine_weyl_group

to doc/en/reference/combinat/module_list.rst to get the documentation into the reference manual. However if we do this we get an error:

OSError: [combinat ] extended_affine_weyl_group.py:docstring of 
ExtendedAffineWeylGroup:11: 
ERROR: The "TOPIC" directive may not be used within topics or body elements.

I don't find TOPIC used very much in the sage documentation. I do find it used in combinat/root_system/plot.py, which is a standalone tutorial. Maybe it can't be used in a docstring. I hoped that changing TOPIC to RUBRIC would work, but I tried it and it doesn't.

I'm leaving the status "needs work" since the documentation needs to compile.

(Maybe the problem is the double colon after TOPIC. I don't have time to try that now.)

Last edited 7 years ago by bump (previous) (diff)

comment:29 Changed 7 years ago by git

  • Commit changed from 66e07a2bb515487a80d8ad494c4ef286d8a181cc to 8ab8135351102f87e07da9a5b347a5643a9fb0a1

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

8ab8135doc cleanup

comment:30 Changed 7 years ago by bump

  • Status changed from needs_work to needs_review

comment:31 Changed 7 years ago by bump

I removed the double colon after TOPIC so that the documentation builds. I also added extended_affine_weyl_group to reference/combinat/module_list and combinat/root_system/init.py.

As far as I know the patch is good now and I've changed the status back to needs review.

Last edited 6 years ago by bump (previous) (diff)

comment:32 Changed 7 years ago by bump

  • Description modified (diff)
  • Status changed from needs_review to needs_work

Since some docstrings don't have doctests I'm changing the status back to "needs work"

Last edited 6 years ago by bump (previous) (diff)

comment:33 Changed 7 years ago by git

  • Commit changed from 8ab8135351102f87e07da9a5b347a5643a9fb0a1 to 737ca2190d7b681df1706bb81af7f605fb3f7383

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

737ca21Merge branch 'develop' into public/combinat/extended_affine_weyl_groups-15375

comment:34 Changed 6 years ago by bump

  • Milestone changed from sage-6.4 to sage-6.7

comment:35 Changed 6 years ago by bump

To facilitate review, here is a link to a build of documentation.

http://sporadic.stanford.edu/reference1/combinat/sage/combinat/root_system/extended_affine_weyl_group.html

comment:36 follow-up: Changed 6 years ago by git

  • Commit changed from 737ca2190d7b681df1706bb81af7f605fb3f7383 to ecfe8e2d164710fdf9957be6c47588cfb9d73cbc

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

ecfe8e2Merge branch 'develop' into public/combinat/extended_affine_weyl_groups-15375

comment:37 in reply to: ↑ 36 ; follow-up: Changed 6 years ago by aschilling

I rebased on the latest development version and will look what else needs to be done to finalize this patch!

Anne

comment:38 in reply to: ↑ 37 Changed 6 years ago by aschilling

Here are some comments on the patch. Mark, can you work on them so we can review them?

  • There are several methods in ExtendedAffineWeylGroup_Class that do not have documentation or tests.
  • I remember that we talked about this at Sage Days 64: can we get rid of to_ambient in all the various places?
  • fundamental_groups.py also does not have full documentation coverage!
  • Also
    pyflakes extended_affine_weyl_group.py
    extended_affine_weyl_group.py:940: undefined name 'Infinity'
    
  • Many methods in sage.groups.group_semidirect_product.py have no documentation.

Best,

Anne

comment:39 Changed 6 years ago by git

  • Commit changed from ecfe8e2d164710fdf9957be6c47588cfb9d73cbc to 95c982263863e58b6aa07a3e10bb16add6c25642

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

95c9822removed some unused imports

comment:40 Changed 6 years ago by git

  • Commit changed from 95c982263863e58b6aa07a3e10bb16add6c25642 to 461ea30fd203504555aed68fa738ed89f6fbc0e3

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

44e4306Merge branch 'public/combinat/extended_affine_weyl_groups-15375' into 6.8.b1
461ea30trac #15375 adding import of Infinity

comment:41 Changed 6 years ago by git

  • Commit changed from 461ea30fd203504555aed68fa738ed89f6fbc0e3 to ea3d56c10c1291520b4dbfd8c59e365203e5a00d

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

5d57171Merge branch 'public/combinat/extended_affine_weyl_groups-15375' into 6.8.b2
ea3d56ctrac #15375 remove duplicate Kac reference

comment:42 Changed 6 years ago by git

  • Commit changed from ea3d56c10c1291520b4dbfd8c59e365203e5a00d to 6ee5258434bfeb7d083ffd0f96f47d1ef41376b6

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

6ee5258trac #15385 py3 form of raise statements

comment:43 Changed 6 years ago by git

  • Commit changed from 6ee5258434bfeb7d083ffd0f96f47d1ef41376b6 to 3835fa9c70161d4765ce76583b2825591bf13888

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

3835fa9trac #15375 forgot two old-style raise

comment:44 follow-up: Changed 6 years ago by git

  • Commit changed from 3835fa9c70161d4765ce76583b2825591bf13888 to d4a052e65c7d950509cb3588d66b5e249c6cca3d

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

1ea823918634 fixed
634a452Merge branch 'extended' into newextended
058c82fMerge branch 'newextended' into extendedJune8
00668e1added doctests for full coverage
886b52aput reflection_to_root and reflection_to_coroot into parent methods
52eac7eMerge branch 'fix_reflection_to_root' into extendedJune8
4a28b14Merge branch 'extendedJune8' into extendedJune9
d4a052ecompleted doctest coverage

comment:45 in reply to: ↑ 44 Changed 6 years ago by aschilling

Hi Mark,

I see that reflection_to_root is changed in this branch. Did you rebase it on top of http://trac.sagemath.org/ticket/18634?

Anne

comment:46 Changed 6 years ago by git

  • Commit changed from d4a052e65c7d950509cb3588d66b5e249c6cca3d to 309c327b244483b650cf7c2ad7a2dc1facf708c0

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

309c327undid the 18635 stuff

comment:47 Changed 6 years ago by git

  • Commit changed from 309c327b244483b650cf7c2ad7a2dc1facf708c0 to 3dab47ad8b0cbac670d035848d2bfd68fd5bb175

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

3dab47a15375: undid changes in categories/weyl_group.py

comment:48 follow-up: Changed 6 years ago by git

  • Commit changed from 3dab47ad8b0cbac670d035848d2bfd68fd5bb175 to 018bf969db55cb76c017ead141f99c22e4cabc25

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

018bf9615375: some more doc fixes

comment:49 in reply to: ↑ 48 Changed 6 years ago by aschilling

Please add what the INPUT is in the following methods in the extended_affine_weyl_group:

apply_simple_projection coset_representative face_data action translation_group_morphism simple_reflection bruhat_le

There are certain methods that occur multiple times with code that looks only minimally different (for example in has_descent it looks like that only left and right have changed). Could you explain why one needs the methods in each realization separately rather than having one general one.

Same in src/sage/groups/group_exp.py. There are methods where some of the input needs to be explained.

comment:50 Changed 6 years ago by aschilling

  • Keywords days65 added

comment:51 Changed 6 years ago by git

  • Commit changed from 018bf969db55cb76c017ead141f99c22e4cabc25 to afc40d7638cdcf20b45119c6ed86205919eb65a9

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

dfa6037doc fixes
afc40d7Merge branch 'public/combinat/extended_affine_weyl_groups-15375' of trac.sagemath.org:sage into extendedJune9

comment:52 Changed 6 years ago by mshimo

  • Status changed from needs_work to needs_review

Since certain methods are much more efficiently computed in certain realizations, the generic implementation just consists of converting to the favorable realization, doing the computation there using a custom implementation, and then converting back. But since converting is often expensive, sometimes a direct algorithm is given for more than one realization. This is the case for :meth:has_descent.

comment:53 follow-up: Changed 6 years ago by git

  • Commit changed from afc40d7638cdcf20b45119c6ed86205919eb65a9 to d146bdd5c5c5d1fbeace1b039c15d63d085cdba4

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

c2efddbMerge branch 'develop' into public/combinat/extended_affine_weyl_groups-15375
d146bddMerge branch 'public/combinat/extended_affine_weyl_groups-15375' of git://trac.sagemath.org/sage into public/combinat/extended_affine_weyl_groups-15375

comment:54 in reply to: ↑ 53 Changed 6 years ago by aschilling

Hi Mark,

I rebased your patch on top of sage-6.8.beta4. So please pull the changes before you keep editing!

I see this in the diff without a doc test and a comment:

diff --git a/src/sage/combinat/root_system/type_affine.py b/src/sage/combinat/root_system/type_affine.py
index f505c47..c19136e 100644
--- a/src/sage/combinat/root_system/type_affine.py
+++ b/src/sage/combinat/root_system/type_affine.py
@@ -419,6 +419,14 @@ class AmbientSpace(CombinatorialFreeModule):
         return vector(list(vector(classical._plot_projection(classical(x)))) +
                       [x["deltacheck"]])
 
+    def from_vector_notation(self, weight, style="lattice"):
+        """
+        ..TODO:: CHECK THIS STUPID DEFAULT IMPLEMENTATION WITH DAN
+
+        This is in particular used to compute demazure-lusztig characters
+        """
+        return weight

Can this be taken out?

When I tried the following, I got an error message

sage: E.inject_shorthands()
---------------------------------------------------------------------------
NotImplementedError                       Traceback (most recent call last)
<ipython-input-28-d05fc29a74b6> in <module>()
----> 1 E.inject_shorthands()

/Applications/sage/local/lib/python2.7/site-packages/sage/categories/sets_cat.pyc in inject_shorthands(self, verbose)
   2351                 from sage.misc.misc import inject_variable
   2352                 if not hasattr(self, "_shorthands"):
-> 2353                     raise NotImplementedError("no shorthands defined for {}".format(self))
   2354                 for shorthand in self._shorthands:
   2355                     realization = getattr(self, shorthand)()

NotImplementedError: no shorthands defined for Extended affine Weyl group of type ['A', 2, 1]

Do you want shorthands for all the realizations?

Similarly,

sage: E.group_generators()
---------------------------------------------------------------------------
NotImplementedError                       Traceback (most recent call last)
<ipython-input-44-d96974f8367c> in <module>()
----> 1 E.group_generators()

/Applications/sage/local/lib/python2.7/site-packages/sage/categories/groups.pyc in group_generators(self)
    108                 return Family(self.gens())
    109             except AttributeError:
--> 110                 raise NotImplementedError("no generators are implemented for this group")
    111 
    112         def monoid_generators(self):

NotImplementedError: no generators are implemented for this group

Best,

Anne

comment:55 Changed 6 years ago by aschilling

  • Milestone changed from sage-6.7 to sage-6.8
  • Reviewers changed from bump to Dan Bump, Anne Schilling
  • Status changed from needs_review to needs_work

comment:56 follow-up: Changed 6 years ago by mshimo

The rebasing weirdness is coming from someone else's code.

For shorthands, the realizations are created by the methods PW0(), W0P(), WF(), FW(), PvW0(), and W0Pv(). Um, do you think this is insufficiently short?

Implementing generators is easy and I can add that.

comment:57 in reply to: ↑ 56 Changed 6 years ago by aschilling

Replying to mshimo:

The rebasing weirdness is coming from someone else's code.

You mean the method from_vector_notation? It looks like it belongs to this patch, but it might be super old. Should we take it out?

For shorthands, the realizations are created by the methods PW0(), W0P(), WF(), FW(), PvW0(), and W0Pv(). Um, do you think this is insufficiently short?

Well, I just tried out methods that E.<tab> offered and many of them are not implemented.

Implementing generators is easy and I can add that.

That would be good!

Dan, could you also do a final review of this patch? I think it is getting close to being ready.

comment:58 Changed 6 years ago by bump

I'll have a look.

comment:59 Changed 6 years ago by git

  • Commit changed from d146bdd5c5c5d1fbeace1b039c15d63d085cdba4 to 870af046ea18ee9530da67918157777807e5b425

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

870af0415375: small change

comment:60 Changed 6 years ago by git

  • Commit changed from 870af046ea18ee9530da67918157777807e5b425 to cb2c7d78ab90a474c4cd320719c101cbbd14b95d

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

cb2c7d7added generators methods

comment:61 Changed 6 years ago by mshimo

The various realizations should now have a generators method.

comment:62 Changed 6 years ago by git

  • Commit changed from cb2c7d78ab90a474c4cd320719c101cbbd14b95d to 9e25def4bd0a2d9b5ad4e654d57d637880da9f7d

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

9e25defimplemented list method for fundamental group

comment:63 follow-up: Changed 6 years ago by mshimo

I added a list method for the fundamental groups. I am not sure that I did this entirely correctly (that is, implementing the correct part of the category framework). I just added a method and added finiteness information to the category.

comment:64 in reply to: ↑ 63 Changed 6 years ago by aschilling

Hi Mark,

Does this address my original question about

sage: E=ExtendedAffineWeylGroup(['A',2,1])
sage: E.group_generators()

or are you fixing something else?

Shall we remove from_vector_notation?

Best,

Anne

comment:65 Changed 6 years ago by git

  • Commit changed from 9e25def4bd0a2d9b5ad4e654d57d637880da9f7d to 4438e0950c8e0758ab961813259a66650fc2b35a

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

4438e09fixed bugs related to finiteness of fundamental group

comment:66 follow-up: Changed 6 years ago by mshimo

Get rid of from_vector_notation.

Group generators are not relevant for E. E is not an actual realization.

E.PW0() is a group and so on.

The list method is related to finiteness (saying "x in G" makes sense if G is a finite group, and iterating over x in G uses the list method). I somehow got on the wrong branch and unfixed some stuff and had to refix it.

comment:67 Changed 6 years ago by mshimo

As far as E.group_generators() is concerned, one could call the default realization of E and invoke its "generators" method.

comment:68 Changed 6 years ago by git

  • Commit changed from 4438e0950c8e0758ab961813259a66650fc2b35a to c021d03217b1f205a577e0a9a1a7f6b1d567789b

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

c021d0315375: removed from_vector_notation

comment:69 in reply to: ↑ 66 Changed 6 years ago by aschilling

Replying to mshimo:

Get rid of from_vector_notation.

I got rid of it in my last commit.

Group generators are not relevant for E. E is not an actual realization.

E.PW0() is a group and so on.

But even for that it does not work for me:

sage: E=ExtendedAffineWeylGroup(['A',2,1])
sage: PW0=E.PW0()
sage: PW0.group_generators()
---------------------------------------------------------------------------
NotImplementedError                       Traceback (most recent call last)
<ipython-input-3-d69de239d91b> in <module>()
----> 1 PW0.group_generators()

/Applications/sage/local/lib/python2.7/site-packages/sage/categories/groups.pyc in group_generators(self)
    108                 return Family(self.gens())
    109             except AttributeError:
--> 110                 raise NotImplementedError("no generators are implemented for this group")
    111 
    112         def monoid_generators(self):

NotImplementedError: no generators are implemented for this group

comment:70 follow-up: Changed 6 years ago by mshimo

Sorry, I added a method "generators" not "group_generators". I think I did this to be consistent with the lattices (such as weight lattices), which use "gens" not "group_generators".

comment:71 in reply to: ↑ 70 Changed 6 years ago by aschilling

Replying to mshimo:

Sorry, I added a method "generators" not "group_generators". I think I did this to be consistent with the lattices (such as weight lattices), which use "gens" not "group_generators".

I would suggest to be consistent with what the category code asks for (in this case group_generators) since this method appears when one hits tab.completion! Otherwise it is confusing to the user.

Other than this, I will leave the final review to Dan!

comment:72 Changed 6 years ago by git

  • Commit changed from c021d03217b1f205a577e0a9a1a7f6b1d567789b to b9152e2bc08cd314744b5fd5ef627a467778f25a

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

addcb09fixed group_generators methods
b9152e2Merge branch 'public/combinat/extended_affine_weyl_groups-15375' of trac.sagemath.org:sage into extended

comment:73 follow-ups: Changed 6 years ago by mshimo

I fixed the group_generators methods. Note that I had to mess with sage.categories.finite_groups.

comment:74 in reply to: ↑ 73 Changed 6 years ago by aschilling

Replying to mshimo:

I fixed the group_generators methods. Note that I had to mess with sage.categories.finite_groups.

Ok, I am satisfied. Dan, could you please do a final review?

comment:75 in reply to: ↑ 73 Changed 6 years ago by nthiery

Replying to mshimo:

Note that I had to mess with sage.categories.finite_groups.

I double checked on this, and it is actually a cleanup. If this does not break tests elsewhere, I am happy with that change. Thanks!

comment:76 Changed 6 years ago by nthiery

Hi Mark!

I had a look at the fundamental group code. Thanks for the hard work!

Suggestions:

  • Would it be possible to make the code more type-free, in particular the initialization? For example, it seems likely to break the current version if one uses a relabeled type BC.
  • The logic for computing the special nodes would fit more naturally in Cartan types rather than here. Please implement that as a method cartan_type.special_nodes().

The special nodes form an orbit under the action of the automorphism group of the Dynkin diagram, right? If yes, then it would be enough to implement a generic method CartanType_affine.special_nodes, which would start from the special node as specified by cartan_type.special_node(), and take its orbit under the automorphism group of the Dynkin diagram. It's a small graph, so that's cheap enough.

  • Maybe add a test that the Cartan type is indeed affine?
  • Is the _element_constructor_ needed at all?
  • lambda i: finite_action_dict[i] -> finite_action_dict.__getitem__
  • What about renaming finite_action_dict to reduced_words?
  • om -> omega (I believe that's what we use in the crystal code)

in general, please avoid abbreviations, unless they correspond to standard one-letter mathematical notations.

  • ..., finite = True) -> ..., finite=True)
  • The code is currently assuming that the 0 is the "main" special node, right? Please use cartan_type.special_node() instead.
  • What about fusing the methods family and group_generators? group_generators should return a family anyway (unlike gens).
  • If you put the group in XXX.FinitelyGenerated(), the list of elements will be recovered from the group generators (of course, here we have a more straightforward way of doing it, so that may, or not, be how we want to do this).

Thanks!

Last edited 6 years ago by nthiery (previous) (diff)

comment:77 follow-up: Changed 6 years ago by mshimo

Nicolas, Thanks very much for your comments.

I need to know the reason behind using cartan_type.special_node().

Let's call this the extra special node. For simplicity we assume untwisted affine type. By deletion we obtain a subsystem of finite type. If the extra special node is nonzero then the correct behavior is that the "classical subsystem" should be a relabeling of the appropriate standard classical subsystem with 0 as one of the "finite" Dynkin nodes. This nonstandard classical Dynkin node set will get propagated to indices of simple roots and fundamental weights and to the finite Weyl groups. In particular we could see t_{\omega_0\vee} as a nontrivial translation element and s_0 as an element of the finite Weyl group. Do we really want this?

--Mark

comment:78 in reply to: ↑ 77 Changed 6 years ago by nthiery

Replying to mshimo:

Nicolas, Thanks very much for your comments.

You are welcome!

I need to know the reason behind using cartan_type.special_node().

Let's call this the extra special node. For simplicity we assume untwisted affine type. By deletion we obtain a subsystem of finite type. If the extra special node is nonzero then the correct behavior is that the "classical subsystem" should be a relabeling of the appropriate standard classical subsystem with 0 as one of the "finite" Dynkin nodes. This nonstandard classical Dynkin node set will get propagated to indices of simple roots and fundamental weights and to the finite Weyl groups. In particular we could see t_{\omega_0\vee} as a nontrivial translation element and s_0 as an element of the finite Weyl group. Do we really want this?

It's indeed nice to have 0 by default for the special node in the canonical labeling of the Dynkin diagram, in order to follow the usual conventions in the literature.

But other than that the code has been designed to be agnostic w.r.t. the labels of the nodes of the Dynkin diagram. This gives the user the flexibility to use his preferred labeling, without imposing a given convention. Also it makes the code robust in potential use cases like the following:

  • The user inputs some Cartan matrix which turns out to be affine. There is no reason a priori that 0 will be a special node. (note: at some point we will need/want to implement the logic to recover some special node).
  • We have a big KM Dynkin diagram, and we want to do some calculations involving a parabolic subgroup. Typically the corresponding Dynkin diagram is disconnected, and the calculation will boil down to the connected pieces. Those connected pieces could be affine Dynkin diagrams, but with labels like 3,4,5; still we might want to use it's fundamental group, etc ...

Cheers,

Nicolas

comment:79 Changed 6 years ago by git

  • Commit changed from b9152e2bc08cd314744b5fd5ef627a467778f25a to f36f4aad0d2d1b38ed0499b19572623c3302deec

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

f36f4aamade fixes suggested by Nicolas; added __iter__ method to fundamental group

comment:80 Changed 6 years ago by mshimo

I added an iter method because without it, doing things like [x for x in F] was not working. I also made F an EnumeratedSet? when it is finite.

comment:81 Changed 6 years ago by mshimo

I mean __iter__; the formatting lost the double underscores.

comment:82 Changed 6 years ago by nthiery

Good point. To avoid having both __iter__ and __list__ you can just do:

def __iter__(self):
    return iter(self.group_generators())

Note the use of iter rather than for ... yield (should be faster).

Also, default list method will already raise an error for you if the set is in XXX.Infinite().

Thanks!

Last edited 6 years ago by nthiery (previous) (diff)

comment:83 Changed 6 years ago by git

  • Commit changed from f36f4aad0d2d1b38ed0499b19572623c3302deec to e7aedfce6156862428c5410f82d43f2b8fc4ccde

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

e7aedfcfixed __iter__ method

comment:84 Changed 6 years ago by git

  • Commit changed from e7aedfce6156862428c5410f82d43f2b8fc4ccde to 93f5adf284de8e73ef973a792a81b11c6d113692

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

93f5adfremoved is_finite method for extended affine Weyl groups and added Finite() and Infinite() information to category

comment:85 Changed 6 years ago by mshimo

I added infinite and finite information to the categories of the extended affine Weyl groups and their fundamental groups. Thanks, Nicolas, for the programming help!

comment:86 Changed 6 years ago by bump

Nicolas, are you satisfied? (Can the status be changed back to needs_review?)

comment:87 Changed 6 years ago by git

  • Commit changed from 93f5adf284de8e73ef973a792a81b11c6d113692 to fd2aa163cf1f5959fa611fec8c131c588e5cfeec

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

fd2aa16little changes

comment:88 Changed 6 years ago by git

  • Commit changed from fd2aa163cf1f5959fa611fec8c131c588e5cfeec to 154656f35ca2fd18f80884117ceeef31fbabfd5d

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

154656flittle changes

comment:89 Changed 6 years ago by git

  • Commit changed from 154656f35ca2fd18f80884117ceeef31fbabfd5d to fee79e7857d9381d9d820022b39cc218d3b185cd

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

fee79e7changed fundamental_group an_element() to give a nonidentity element when possible; had to fix the doctests

comment:90 Changed 6 years ago by mshimo

I think I am done messing with this ticket for now. I added an explicit an_element method; the default one always gives the identity, which leads to less interesting example elements for doctests. So I did this and fixed the doctests.

comment:91 Changed 6 years ago by git

  • Commit changed from fee79e7857d9381d9d820022b39cc218d3b185cd to cece2fbe6b3623231dce2cbcd28be8f561f782a4

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

cece2fblittle fix to family method of GL fundamental group and added a group_generators method for it

comment:92 Changed 6 years ago by git

  • Commit changed from cece2fbe6b3623231dce2cbcd28be8f561f782a4 to 2b78b7bdc2db6835dadfad978e5f6992f0546649

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

2b78b7bMerge branch 'develop' into public/combinat/extended_affine_weyl_groups-15375

comment:93 Changed 6 years ago by aschilling

  • Status changed from needs_work to needs_review

comment:94 Changed 6 years ago by nthiery

Hi Mark!

I have glanced half way through the patch. It's a lot of hard work you did; thanks! I'll try to go through the over half soon, but won't have the time for a systematic review, so please someone!

Here are some thoughts that popped up; take them as they just are: random food for thoughts. If something does not sound applicable, just ignore it.

  • weyl_groups.py line 420,453: any reason to remove the test for s2s1s2s1?
  • RootSystem(XXX.cartan_type()) -> XXX.cartan_type().root_system()
  • def to_ambient_space_morphism: you can use sage.misc.c3_controlled.identity. Yeah, there is no reason why the identity function is defined in this module; but at least if we all use the same identity function it will be easier to refactor later on.

Variants, if you want a morphism: End(self).identity() or `sage: self.coerce_map_from(self)`,

  • ..warning -> .. WARNING
  • In to_weight_space: instead of going through a vector, one can use .sum_of_terms([i,self.scalar(alpha[i])] for ...

This method is generic and would better be in WeightSpaceRealizations.

  • to_ambient
  • special_nodes: any reason not to use return autos.orbit(self.special_node()) ?
  • Nicolas Thiery -> Nicolas M. Thiery for consistency
  • Doc of ExtendedAffineWeylGroup: use .. MATH for the math formulas on their own line.
  • ... the following lattices:; idem for E below.
  • When natural, put the :: at the end of the sentence.
  • Line folding could be more consistent
  • There are still quite a few of ifs depending on twisted/untwisted/GL; any chance to make this more generic?

For example, to define the special translation covector can't we generically use something like `c[special_node] * _special_root.associated_coroot()`? I never remember from the top of my head whether it's c or one of its friends that should be used here, but we did things like this in the MacDo? code.

Note that at this point the initialization of ExtendedAffineWeylGroup_class will probably break on a relabelled type BC.

  • Do we need the special_nodes method on the group and the fundamental group? Or could just have it on the cartan type?
  • Strip new lines just before and just after the end of the documentation string.
  • classical_weyl -> classical? or classical_weyl_group? I don't remember what we did in similar situations for crystals and the like, but please check for consistency.
  • classical_weyl_to_affine_morphism: the name is misleading since the result is not a morphism. Could we have a name like from_classical or make this an element method to_affine? Same for the related methods.
  • cardinality is already provided by Sets().Infinite()
  • group_generators this could in principle go in Groups().WithRealizations(). If creating the latter just for this seems like overkill, you can just add a comment about this.
  • ...Realizations.ParentMethods.one: replacing PW0 by a_realization the code would be generic and could go in Magmas.Unital.Realizations.
  • Returns -> Return
  • coset_representative, is_grassmanian, ...: this is the same logic as for a usual coxeter group, right? Could we avoid the duplication by having a common super category?
  • get_the_index -> leading_support?
  • FundamentalGroup...one: 0 -> special_node
  • an_element: you can probably use .last() (haven't tested)
  • action: maybe representation? Elsewhere in Sage, action is usually a method that implements the action itself.
  • root_lattice_realizations: Algebras is now imported twice.

Cheers,

Nicolas

comment:95 Changed 6 years ago by tscrim

For Cartan types, it is simply classical and for crystals, it is classical_object (ex. classical_weight).

comment:96 follow-up: Changed 6 years ago by bump

I looked at the patchbot shortlogs for the first three machines reporting build failure. They all report the same error:

OSError: [combinat ] /home/worker/sage-patchbot/local/lib/python2.7/site-packages/sage/combinat/root_system/extended_affine_weyl_group.py:docstring of sage.combinat.root_system.extended_affine_weyl_group.ExtendedAffineWeylGroup_Class.WF_to_PW0_func:11: ERROR: Content block expected for the "WARNING" directive; none found.

I think the "Since this is used ..." at line 1068 needs to be indented. Other bots are reporting a doctest error in dev_tools.py.

**********************************************************************
File "src/sage/misc/dev_tools.py", line 459, in sage.misc.dev_tools.import_statements
Failed example:
    load_submodules(sage.sets)
Expected:
    load sage.sets.cartesian_product... succeeded
    load sage.sets.set_from_iterator... succeeded
Got:
    load sage.sets.real_set... succeeded
    load sage.sets.set_from_iterator... succeeded
**********************************************************************

I suspect this is unrelated to this patch. It may be caused by #18553 which was merged three weeks ago and which touched sage.sets.real_set.py.

However I think the above docstring needs to be fixed and since Nicolas has some comments I'm changing the status back to needs_work.

comment:97 Changed 6 years ago by bump

  • Status changed from needs_review to needs_work

comment:98 Changed 6 years ago by git

  • Commit changed from 2b78b7bdc2db6835dadfad978e5f6992f0546649 to 74c7d9edc80c6438d184979f0b06e6980b4ee196

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

c0c7c0fsomehow bugfix of trac 18634 was undone. I redid it.
a06f404some changes Nicolas suggested
79d4fe5fixing doc strings and implementing more suggestions from Nicolas
ba64e29more little fixes
74c7d9eMerge branch 't/15375/public/combinat/extended_affine_weyl_groups-15375' into extended

comment:99 Changed 6 years ago by tscrim

The one() method added to unital magmas will need a doctest.

comment:100 Changed 6 years ago by mshimo

  • Branch changed from public/combinat/extended_affine_weyl_groups-15375 to ba64e29
  • Commit 74c7d9edc80c6438d184979f0b06e6980b4ee196 deleted

It wouldn't let me push the fixed version so I made a copy of the existing ticket branch and merged it into my working branch. The result is broken. I am going to try to change the trac branch to a good one.


New commits:

c0c7c0fsomehow bugfix of trac 18634 was undone. I redid it.
a06f404some changes Nicolas suggested
79d4fe5fixing doc strings and implementing more suggestions from Nicolas
ba64e29more little fixes
74c7d9eMerge branch 't/15375/public/combinat/extended_affine_weyl_groups-15375' into extended

comment:101 Changed 6 years ago by mshimo

  • Branch changed from ba64e29 to u/mshimo/combinat/extended_affine_weyl_groups-15375
  • Commit set to c36eb6bd081b55b3785294ccff9c496e592707eb

Last 10 new commits:

93f5adfremoved is_finite method for extended affine Weyl groups and added Finite() and Infinite() information to category
fd2aa16little changes
154656flittle changes
fee79e7changed fundamental_group an_element() to give a nonidentity element when possible; had to fix the doctests
cece2fblittle fix to family method of GL fundamental group and added a group_generators method for it
c0c7c0fsomehow bugfix of trac 18634 was undone. I redid it.
a06f404some changes Nicolas suggested
79d4fe5fixing doc strings and implementing more suggestions from Nicolas
ba64e29more little fixes
c36eb6badded doctest to magma

comment:102 follow-up: Changed 6 years ago by mshimo

Ugh. I clearly don't know how to use trac.

comment:103 in reply to: ↑ 102 Changed 6 years ago by aschilling

Replying to mshimo:

Ugh. I clearly don't know how to use trac.

What happened? It might happen that one cannot push if there is a change on the server that one has not pulled yet. Otherwise I would not know why this happens.

I get the following failure in sage/categories

sage -t realizations.py
**********************************************************************
File "realizations.py", line 44, in sage.categories.realizations.RealizationsCategory
Failed example:
    Groups().Realizations().super_categories()
Expected:
    [Category of groups, Category of realizations of magmas]
Got:
    [Category of groups, Category of realizations of unital magmas]
**********************************************************************
File "realizations.py", line 68, in sage.categories.realizations.Realizations
Failed example:
    Algebras(QQ).Realizations()
Expected:
    Join of Category of algebras over Rational Field and Category of realizations of magmas
Got:
    Join of Category of algebras over Rational Field and Category of realizations of unital magmas
**********************************************************************
2 items had failures:
   1 of   7 in sage.categories.realizations.Realizations
   1 of   5 in sage.categories.realizations.RealizationsCategory
    [27 tests, 2 failures, 0.10 s]

comment:104 Changed 6 years ago by bump

There are currently doctests failures in categories/realizations.py and categories/examples/with_realizations.py. These would be fixed by correcting the doctest, viz. replacing 'magmas' by 'unital magmas' in the failing docutests.

There is also a doctest failure in misc/dev_tools.py. In comment:96 I speculated that this was not caused by this patch, but evidently this was wrong. I don't know what caused this doctest to change, but I checked out the git commit 05875d563029eb7231d2cae4b434b99e4742fbbf and confirm that dev_tools tests pass for that version. Currently this patch is rebased atop that commit and I think this proves that it caused the failure. I don't know the mechanism but the test in question seems to be pretty fragile.

There is also a build failure patchbot report (as before) that a WARNING directive is missing a content block. It looks to me as if Mark's changes should have fixed this (the content block is now indented). So I'm puzzled by this (unless the build attempt predated Mark's commit. It is date stamped 6/20.)

comment:105 Changed 6 years ago by git

  • Commit changed from c36eb6bd081b55b3785294ccff9c496e592707eb to dde0d4e57f9473f99c8428f378c8a1fd46c23325

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

dde0d4efixed magma doctests

comment:106 Changed 6 years ago by mshimo

Nicolas, aren't you the author of dev_tools? Do you know what is going on with the doctest?

comment:107 Changed 6 years ago by bump

I noticed that the dev_tools doctest that fails is fragile: if you call it from the command line it gives a different result than the doctest.

sage: from sage.misc.dev_tools import load_submodules
sage: load_submodules(sage.sets)
load sage.sets.cartesian_product... succeeded
load sage.sets.real_set... succeeded
load sage.sets.set_from_iterator... succeeded

Presumably this is because when the doctest is run, sage.sets.real_set has already been loaded.

Since the branch was recently changed to u/mshimo I think Mark is the only one who can make changes now. When it was u/public others could push changes, for example when Anne rebased it.

Last edited 6 years ago by bump (previous) (diff)

comment:108 follow-up: Changed 6 years ago by mshimo

This is part of my misunderstanding of trac.

I had trouble pushing to the u/public branch; it was forcing a merge which was creating weird errors that didn't happen in my local branch. That is why I made a new copy and put it in a new location, apparently unwittingly making it nonpublic.

I would like to just clobber (replace) the old u/public branch with the other one. Can I do that? Is it a bad idea? I just know that my previous attempts to push to that branch, created weird nonrecognizable errors that were not my code's fault.

The alternative is to make a new public branch.

comment:109 in reply to: ↑ 108 Changed 6 years ago by aschilling

Replying to mshimo:

This is part of my misunderstanding of trac.

I had trouble pushing to the u/public branch; it was forcing a merge which was creating weird errors that didn't happen in my local branch. That is why I made a new copy and put it in a new location, apparently unwittingly making it nonpublic.

I would like to just clobber (replace) the old u/public branch with the other one. Can I do that? Is it a bad idea? I just know that my previous attempts to push to that branch, created weird nonrecognizable errors that were not my code's fault.

The alternative is to make a new public branch.

You should be able to push your local branch to a public one by saying

git branch <localname> --set-upstream-to origin/<newremotename>

where <newremotename> is something like public/15375 (pick a name different from the old public name just in case). Then you can copy the new branch name in the trac field on the ticket.

comment:110 Changed 6 years ago by mshimo

  • Branch changed from u/mshimo/combinat/extended_affine_weyl_groups-15375 to public/15375

comment:111 in reply to: ↑ 96 ; follow-up: Changed 6 years ago by nthiery

Replying to bump:

Other bots are reporting a doctest error in dev_tools.py.

**********************************************************************
File "src/sage/misc/dev_tools.py", line 459, in sage.misc.dev_tools.import_statements
Failed example:
    load_submodules(sage.sets)
Expected:
    load sage.sets.cartesian_product... succeeded
    load sage.sets.set_from_iterator... succeeded
Got:
    load sage.sets.real_set... succeeded
    load sage.sets.set_from_iterator... succeeded
**********************************************************************

I suspect this is unrelated to this patch. It may be caused by #18553 which was merged three weeks ago and which touched sage.sets.real_set.py.

My current bet is that this ticket somehow forces sage.sets.cartesian_product to be loaded upon Sage's startup. Hence, when running load_submodules(sage.sets) it does not appear anymore as the first entry, and instead the output starts with the next one which turns out to be sage.sets.real_set.

If that's confirmed, a good approach would be to make sure that all the affine Weyl group code is lazy imported. That's a good property to have anyway.

Cheers,

Nicolas

comment:112 Changed 6 years ago by git

  • Commit changed from dde0d4e57f9473f99c8428f378c8a1fd46c23325 to e0943139b6b741cd5e4030ae99a25e2f28ccaed9

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

e094313made extended affine weyl group into a lazy import

comment:113 Changed 6 years ago by mshimo

The new module sage.categories.groups.group_semidirect_product does import CartesianProduct?. Would that make a difference?


New commits:

e094313made extended affine weyl group into a lazy import

comment:114 in reply to: ↑ 111 Changed 6 years ago by bump

Replying to nthiery:

If that's confirmed, a good approach would be to make sure that all the affine Weyl group code is lazy imported. That's a good property to have anyway.

Mark's last commit did change to lazy import and did not fix the doctest.

My current bet is that this ticket somehow forces sage.sets.cartesian_product to be loaded upon Sage's startup.

I think this may not have to do with what is loaded on startup, but rather what is loaded during other tests coming earlier in the docstring. If this test is moved to the top of the docstring, it will give a different result. See comment:107.

comment:115 Changed 6 years ago by git

  • Commit changed from e0943139b6b741cd5e4030ae99a25e2f28ccaed9 to ee155e445baa2c406162df421ad8ad9b10d4292d

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

ee155e4just changed doctest in load_submodules to give the "right" answer

comment:116 Changed 6 years ago by mshimo

As you can see from the commit message, I just changed the answer in the doctest to the answer generated by the current version of sage.

comment:117 follow-up: Changed 6 years ago by bump

  • Status changed from needs_work to needs_review

On my machine it builds and all tests pass. But two buildbots are failing to build with cython errors.

Surely this can have nothing to do with this patch.

I'm changing the status to needs_review. I hesitate to change it to positive_review until the patchbot is happy.

comment:118 in reply to: ↑ 117 Changed 6 years ago by aschilling

Replying to bump:

On my machine it builds and all tests pass. But two buildbots are failing to build with cython errors.

Surely this can have nothing to do with this patch.

I'm changing the status to needs_review. I hesitate to change it to positive_review until the patchbot is happy.

For me everything builds fine. I ran sage -testall and got the following failures

sage -t src/sage/groups/perm_gps/permgroup_named.py
**********************************************************************
File "src/sage/groups/perm_gps/permgroup_named.py", line 2609, in sage.groups.perm_gps.permgroup_named.PSL.ramification_module_decomposition_hurwitz_curve
Failed example:
    G.ramification_module_decomposition_hurwitz_curve() # random, optional - database_gap
Exception raised:
    Traceback (most recent call last):
      File "/Applications/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 496, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/Applications/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 858, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.groups.perm_gps.permgroup_named.PSL.ramification_module_decomposition_hurwitz_curve[1]>", line 1, in <module>
        G.ramification_module_decomposition_hurwitz_curve() # random, optional - database_gap
      File "/Applications/sage/local/lib/python2.7/site-packages/sage/groups/perm_gps/permgroup_named.py", line 2630, in ramification_module_decomposition_hurwitz_curve
        mults = gap.eval("ram_module_hurwitz("+str(q)+")")
      File "/Applications/sage/local/lib/python2.7/site-packages/sage/interfaces/gap.py", line 568, in eval
        result = Expect.eval(self, input_line, **kwds)
      File "/Applications/sage/local/lib/python2.7/site-packages/sage/interfaces/expect.py", line 1239, in eval
        for L in code.split('\n') if L != ''])
      File "/Applications/sage/local/lib/python2.7/site-packages/sage/interfaces/gap.py", line 766, in _eval_line
        raise RuntimeError(message)
    RuntimeError: Gap produced error output
    Error, sorry, the GAP Tables Of Marks Library is not installed

       executing ram_module_hurwitz(13);
**********************************************************************
File "src/sage/groups/perm_gps/permgroup_named.py", line 2657, in sage.groups.perm_gps.permgroup_named.PSL.ramification_module_decomposition_modular_curve
Failed example:
    G.ramification_module_decomposition_modular_curve() # random, optional - database_gap
Exception raised:
    Traceback (most recent call last):
      File "/Applications/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 496, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/Applications/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 858, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.groups.perm_gps.permgroup_named.PSL.ramification_module_decomposition_modular_curve[1]>", line 1, in <module>
        G.ramification_module_decomposition_modular_curve() # random, optional - database_gap
      File "/Applications/sage/local/lib/python2.7/site-packages/sage/groups/perm_gps/permgroup_named.py", line 2674, in ramification_module_decomposition_modular_curve
        mults = gap.eval("ram_module_X("+str(q)+")")
      File "/Applications/sage/local/lib/python2.7/site-packages/sage/interfaces/gap.py", line 568, in eval
        result = Expect.eval(self, input_line, **kwds)
      File "/Applications/sage/local/lib/python2.7/site-packages/sage/interfaces/expect.py", line 1239, in eval
        for L in code.split('\n') if L != ''])
      File "/Applications/sage/local/lib/python2.7/site-packages/sage/interfaces/gap.py", line 766, in _eval_line
        raise RuntimeError(message)
    RuntimeError: Gap produced error output
    Error, sorry, the GAP Tables Of Marks Library is not installed

       executing ram_module_X(7);
**********************************************************************
2 items had failures:
   1 of   3 in sage.groups.perm_gps.permgroup_named.PSL.ramification_module_decomposition_hurwitz_curve
   1 of   3 in sage.groups.perm_gps.permgroup_named.PSL.ramification_module_decomposition_modular_curve
    [439 tests, 2 failures, 3.16 s]
----------------------------------------------------------------------
sage -t src/sage/groups/perm_gps/permgroup_named.py  # 2 doctests failed
----------------------------------------------------------------------
Total time for all tests: 3.3 seconds
    cpu time: 2.3 seconds
    cumulative wall time: 3.2 seconds
sage -t src/sage/tests/gap_packages.py
**********************************************************************
File "src/sage/tests/gap_packages.py", line 15, in sage.tests.gap_packages
Failed example:
    test_packages(['atlasrep', 'tomlib'])    # optional - database_gap
Expected:
      Status   Package    GAP Output
    +--------+----------+------------+
               atlasrep   true
               tomlib     true
Got:
      Status    Package    GAP Output
    +---------+----------+------------+
      Failure   atlasrep   fail
      Failure   tomlib     fail
**********************************************************************
1 item had failures:
   1 of   4 in sage.tests.gap_packages
    [9 tests, 1 failure, 0.18 s]
----------------------------------------------------------------------
sage -t src/sage/tests/gap_packages.py  # 1 doctest failed
----------------------------------------------------------------------
Total time for all tests: 0.2 seconds
    cpu time: 0.2 seconds
    cumulative wall time: 0.2 seconds

I do not think they are related to this branch. So unless there is something else, I guess we can set positive review.

comment:119 Changed 6 years ago by tscrim

In decreasing probability, it looks like your database_gap optional spkg is out of date, mis-/not installed, or needs optional GAP packages installed directly into GAP. I'd try running sage -i database_gap and sage -i gap_packages and then running those tests again to see if they work.

Last edited 6 years ago by tscrim (previous) (diff)

comment:120 follow-up: Changed 6 years ago by bump

Any conjectures about the patchbot?

I presume that the patchbot merges the patch on top of the develop tip then tries to build. Is that correct? It looks from the log as if it actually tries to merge it with a branch called patchbot/base. There doesn't actually seem to be such a branch in the repository.

comment:121 in reply to: ↑ 120 Changed 6 years ago by aschilling

Thanks Travis, that did the trick. For me all tests pass now.

Replying to bump:

Any conjectures about the patchbot?

I presume that the patchbot merges the patch on top of the develop tip then tries to build. Is that correct? It looks from the log as if it actually tries to merge it with a branch called patchbot/base. There doesn't actually seem to be such a branch in the repository.

On my local computer, I merged in the latest development version sage-6.8.beta6 and everything merged fine. So I think something is wrong with the patchbot.

I am going to set positive review, since we all seem happy with the patch.

comment:122 Changed 6 years ago by bump

Right, I ran all the tests yesterday after a distclean build and there were no failures.

I am going to set positive review, since we all seem happy with the patch.

comment:123 Changed 6 years ago by aschilling

  • Status changed from needs_review to positive_review

comment:124 Changed 6 years ago by vbraun

  • Status changed from positive_review to needs_work
l.165775 \end{gather}
                     \paragraph{Fundamental group of affine Dynkin automorph...

? 
! Emergency stop.
\math@cr@@@ ...@ \@ne \add@amps \maxfields@ \omit 
                                                  \kern -\alignsep@ \iftag@ ...
l.165775 \end{gather}
                     \paragraph{Fundamental group of affine Dynkin automorph...

!  ==> Fatal error occurred, no output PDF file produced!
Transcript written on combinat.log.
Makefile:55: recipe for target 'combinat.pdf' failed
make[2]: *** [combinat.pdf] Error 1

comment:125 Changed 6 years ago by mshimo

The build of pdf documentation fails. It outputs algebra.pdf and then seems to hang. I have no idea what is going on. I didn't see a log file generated for this.

comment:126 Changed 6 years ago by jdemeyer

Now that this ticket needs_work anyway:

In the 4 files that you're adding in this ticket: please use the standard copyright statement as explained in http://doc.sagemath.org/html/en/developer/coding_basics.html#headings-of-sage-library-code-files

Currently, two of the files have no copyright statement and two do not state the GPL version.

comment:127 Changed 6 years ago by bump

Here is a log of the results of `sage --docbuild reference pdf`

http://match.stanford.edu/pdflog

(Temporary URL)

comment:128 Changed 6 years ago by jhpalmieri

By default, a .. MATH:: block is automatically enclosed in a math environment, so you shouldn't use an environment like align* there: it gets turned into something like

\begin{gather}
\begin{align*}
...
\end{align*}
\end{gather}

which is not valid LaTeX. To avoid the default, you can add :nowrap: at the beginning of the math block, or you can use an environment like aligned instead. This will fix it, but I would instead choose only one of these two approaches (instead of switching between them, as I've done here):

  • src/sage/combinat/root_system/extended_affine_weyl_group.py

    diff --git a/src/sage/combinat/root_system/extended_affine_weyl_group.py b/src/sage/combinat/root_system/extended_affine_weyl_group.py
    index 7d35cac..56d9e6d 100644
    a b def ExtendedAffineWeylGroup(cartan_type, general_linear=None, **print_options): 
    155155    There are isomorphisms
    156156
    157157    .. MATH::
     158       :nowrap:
    158159
    159160        \begin{align*}
    160161            W &\cong M \rtimes W_0 \cong W_0 \ltimes M \\
  • src/sage/groups/group_semidirect_product.py

    diff --git a/src/sage/groups/group_semidirect_product.py b/src/sage/groups/group_semidirect_product.py
    index 627056f..63b8769 100644
    a b class GroupSemidirectProduct(CartesianProduct): 
    152152       
    153153    .. MATH::
    154154
    155         \begin{align*}
     155        \begin{aligned}
    156156        (g_1,h_1)(g_2,h_2) &= g_1 h_1 g_2 h_2 \\
    157157        &= g_1 g_2 g_2^{-1} h_1 g_2 h_2 \\
    158158        &= (g_1g_2, twist(g_2^{-1}, h_1) h_2)
    159         \end{align*}
     159        \end{aligned}
    160160
    161161    If ``act_to_right`` is False, the group `G \rtimes H` is specified by a homomorphism `\psi\in \mathrm{Hom}(H,\mathrm{Aut}(G))`
    162162    such that
    class GroupSemidirectProduct(CartesianProduct): 
    175175
    176176    .. MATH::
    177177
    178         \begin{align*}
     178        \begin{aligned}
    179179        (g_1,h_1)(g_2,h_2) &= g_1 h_1 g_2 h_2 \\
    180180        &= g_1 h_1 g_2 h_1^{-1} h_1 h_2 \\
    181181        &= (g_1 twist(h_1,g_2), h_1 h_2)
    182         \end{align*}
     182        \end{aligned}
    183183
    184184    If ``prefix0`` (resp. ``prefixl``) is not None then it is used as a wrapper for
    185185    printing elements of ``G`` (resp. ``H``). If ``print_tuple`` is True then elements are printed

comment:129 Changed 6 years ago by bump

There is a huge file called `doc/output/latex/en/reference/combinat/` and at line 165376 we have the following:

There are isomorphisms
\begin{gather}
\begin{split}\begin{align*}
    W &\cong M \rtimes W_0 \cong W_0 \ltimes M \\
    E &\cong L \rtimes W_0 \cong W_0 \ltimes L
\end{align*}\end{split}\notag
\end{gather}\paragraph{Fundamental group of affine Dynkin automorphisms}

At this point pdflatex barfs.

comment:130 Changed 6 years ago by mshimo

Thanks, Dan, that is very helpful. I will fix the doc.

comment:131 Changed 6 years ago by tscrim

As John had mentioned, you need to change:

-        \begin{align*}
+        \begin{aligned}
             W &\cong M \rtimes W_0 \cong W_0 \ltimes M \\
             E &\cong L \rtimes W_0 \cong W_0 \ltimes L
-        \end{align*}
+        \begin{aligned}

and

     .. MATH::
 
-        \begin{align*}
+        \begin{aligned}
         (g_1,h_1)(g_2,h_2) &= g_1 h_1 g_2 h_2 \\
         &= g_1 g_2 g_2^{-1} h_1 g_2 h_2 \\
         &= (g_1g_2, twist(g_2^{-1}, h_1) h_2)
-        \end{align*}
+        \end{aligned}

Also I believe this needs to be changed (there are 2 of theses):

     .. MATH::
 
-        g h g^{-1} &= \phi(g)(h).
+        g h g^{-1} = \phi(g)(h).

comment:132 Changed 6 years ago by git

  • Commit changed from ee155e445baa2c406162df421ad8ad9b10d4292d to f54934489e6183a647a9195d277a1a9344ac831b

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

f549344fixed doc errors

comment:133 Changed 6 years ago by bump

  • Status changed from needs_work to positive_review

Sage now builds, reference.pdf builds and all tests pass. I'm setting status back to positive review.

comment:134 Changed 6 years ago by vbraun

  • Branch changed from public/15375 to f54934489e6183a647a9195d277a1a9344ac831b
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:135 Changed 6 years ago by jdemeyer

  • Authors changed from Daniel Bump, Dan Orr, Anne Schilling, Mark Shimozono, Nicolas Thiery. to Daniel Bump, Dan Orr, Anne Schilling, Mark Shimozono, Nicolas M. Thiéry
  • Commit f54934489e6183a647a9195d277a1a9344ac831b deleted

comment:136 Changed 6 years ago by kcrisman

  • Authors changed from Daniel Bump, Dan Orr, Anne Schilling, Mark Shimozono, Nicolas M. Thiéry to Dan Bump, Dan Orr, Anne Schilling, Mark Shimozono, Nicolas M. Thiéry

comment:137 Changed 5 years ago by chapoton

  • Authors changed from Dan Bump, Dan Orr, Anne Schilling, Mark Shimozono, Nicolas M. Thiéry to Daniel Bump, Dan Orr, Anne Schilling, Mark Shimozono, Nicolas M. Thiéry
Note: See TracTickets for help on using tickets.