Ticket #8414 (closed defect: fixed)
lattice -> domain in weyl_groups.py
| Reported by: | bump | Owned by: | bump |
|---|---|---|---|
| Priority: | major | Milestone: | sage-4.4 |
| Component: | combinatorics | Keywords: | Weyl groups |
| Cc: | sage-combinat | Work issues: | |
| Report Upstream: | N/A | Reviewers: | Nicolas M. Thiéry |
| Authors: | Daniel Bump | Merged in: | sage-4.4.alpha1 |
| Dependencies: | Stopgaps: |
Description (last modified by bump) (diff)
WeylGroups? and WeylGroupElements? have a method lattice() and also an attribute _lattice. At one time this pointed to the ambient lattice, but now it points to the ambient space.
After some discussion here:
it has been decided that the method and attribute should be called domain.
The patch also makes reflections of the Weyl group a family and adds methods inverse_family and has_key to the method family, per Nicolas' suggestion.
Attachments
Change History
comment:1 Changed 3 years ago by bump
- Owner changed from AlexGhitza to bump
- Component changed from algebra to combinatorics
comment:4 Changed 3 years ago by nthiery
- Status changed from needs_review to needs_work
Hi Dan!
Sorry for the late reply; I am just back from vacations.
Depending on how the Weyl group is constructed, W.lattice() can actually be a lattice:
sage: WeylGroup(RootSystem(["A",3]).root_lattice()) Weyl Group of type ['A', 3] (as a matrix group acting on the root lattice) sage: WeylGroup(RootSystem(["A",3]).root_lattice()).lattice() Root lattice of the Root system of type ['A', 3]
In fact, the name was meant as short hand for "which realization of the root lattice the group is naturally acting upon".
That being said, I agree that this name is not good. In particular, we probably will want to generalize this soon to handle Coxeter groups implemented as permutation groups (e.g. acting on the roots) instead of matrix groups. So the semantic of this method should probably be to return which ever natural space (or set) the group is naturally acting on. Its name should reflect that.
Any good suggestions?
- natural_action_space?
- root_system_realization?
- ...?
As for the reflections: this sounds like a useful feature, thanks! May I suggest an alternative implementation, namely to:
- make reflections to be a family (still with the roots as keys) instead of a dictionary
- Add an "inverse" feature to finite families (at least those without duplicates) returning the family with the keys and values exchanged.
Then, you would do W.reflections().inverse_family() instead of W.reflections(keys=reflections). This would solve the problem at hand, be of general usefulness, and not clutter the Weyl group interface.
Thanks in advance!
Best,
Nicolas
comment:5 Changed 3 years ago by bump
That being said, I agree that this name is not good. In particular, we probably will want to generalize this soon to handle Coxeter groups implemented as permutation groups (e.g. acting on the roots) instead of matrix groups. So the semantic of this method should probably be to return which ever natural space (or set) the group is naturally acting on. Its name should reflect that.
It seems to me that the need to implement roots for general Coxeter groups is a distinct issue. If the Coxeter group happens to be a Weyl group the roots are embedded in a lattice or vector space and that is a sufficiently important special case that it should be preserved.
Any good suggestions?
* natural_action_space?
* root_system_realization?
* ...?
To me it would seem best to call it space. Then if the Weyl group is created in such a way that it is a lattice, it would be a misnomer, but calling a lattice a space seems less egregious than calling a space a lattice.
An alternative term would be *module*.
I will revise the patch implementing the change for families if we can agree on this matter of terminology.
Dan
comment:6 Changed 3 years ago by bump
- Status changed from needs_work to needs_review
- Description modified (diff)
I have revised the patch taking into account Nicolas' suggestion that reflections be a family, and that finite families should have inverse families as a method.
I did not revise the change lattice -> space pending further discussion of the matter. As I said in my last message, it seems that it might sometimes be a misnomer but still an improvement over calling the method "lattice".
comment:7 follow-up: ↓ 8 Changed 3 years ago by bump
- Description modified (diff)
- Summary changed from lattice -> space in weyl_groups.py to lattice -> domain in weyl_groups.py
comment:8 in reply to: ↑ 7 ; follow-up: ↓ 9 Changed 3 years ago by hivert
- Status changed from needs_review to needs_work
- Work issues set to doc coverage
Hi Dan,
I had a quick look at your patch. It looks good but I'm not sure I'll find time to formally review it so if someone else wants to formally review it please go. I just want to mention a small problem which forbid it to go into sage: there is no examples or doctests for inverse_family. Moreover I think that has_key probably deserve a negative (i.e. returning False) examples.
Sorry for not having the time to do it...
Florent
comment:9 in reply to: ↑ 8 Changed 3 years ago by nthiery
Replying to hivert:
Hi Dan,
I had a quick look at your patch. It looks good but I'm not sure I'll find time to formally review it so if someone else wants to formally review it please go. I just want to mention a small problem which forbid it to go into sage: there is no examples or doctests for inverse_family. Moreover I think that has_key probably deserve a negative (i.e. returning False) examples.
I have fixed those yesterday in the Sage-Combinat queue; I'll try to finish the review tonight.
Cheers,
comment:10 Changed 3 years ago by nthiery
- Keywords Weyl groups added
- Reviewers set to Nicolas M. Thiéry
- Status changed from needs_work to needs_review
- Work issues doc coverage deleted
Hi Dan!
All test pass, and the patch does what we had agreed upon. Thanks for handling this!
Please double check my reviewer patch, and if ok set a positive review on my behalf!
comment:11 Changed 3 years ago by bump
- Status changed from needs_review to positive_review
OK, I am setting positive review on the reviewer's patch.
comment:12 Changed 3 years ago by jhpalmieri
Dan: please produce patch files using Mercurial, not using diff: they should have a header listing your email address and other information. The "commit" message should also start with the trac ticket, also. See http://www.sagemath.org/doc/developer/walk_through.html#submitting-a-change for more information.
comment:13 follow-up: ↓ 14 Changed 3 years ago by bump
Responding to jhpalmieri, I remade my patch to contain mercurial headers.
Nicolas' patch goes on top of mine. His patch headers contain some non-ascii characters which I had to delete before merging his patch.
comment:14 in reply to: ↑ 13 Changed 3 years ago by nthiery
Replying to bump:
Responding to jhpalmieri, I remade my patch to contain mercurial headers.
Please also include #8414: in front of the patch description!
Nicolas' patch goes on top of mine. His patch headers contain some non-ascii characters which I had to delete before merging his patch.
Oops, mercurial now speaks French on my machine????
I just re=uploaded the patch after fixing it.
Changed 3 years ago by bump
-
attachment
trac_8414_weyl_group_space.patch
added
Rename lattice() method space() in WeylGroups?. Add keys option to reflections()
comment:15 Changed 3 years ago by bump
comment:16 Changed 3 years ago by jhpalmieri
- Status changed from positive_review to closed
- Resolution set to fixed
- Merged in set to sage-4.4.alpha1
Merged in 4.4.alpha1:
- trac_8414_weyl_group_space.patch
- trac_8414_weyl_group_space-review-nt.patch
