Ticket #8414 (closed defect: fixed)

Opened 3 years ago

Last modified 3 years ago

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:

 http://groups.google.com/group/sage-devel/browse_thread/thread/ad0c77557e78313f/9cfd6f09bcd1de2f?#9cfd6f09bcd1de2f

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

trac_8414_weyl_group_space-review-nt.patch Download (5.3 KB) - added by nthiery 3 years ago.
trac_8414_weyl_group_space.patch Download (14.6 KB) - added by bump 3 years ago.
Rename lattice() method space() in WeylGroups?. Add keys option to reflections()

Change History

comment:1 Changed 3 years ago by bump

  • Owner changed from AlexGhitza to bump
  • Component changed from algebra to combinatorics

comment:2 Changed 3 years ago by bump

  • Description modified (diff)

comment:3 Changed 3 years ago by bump

  • Status changed from new to needs_review

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.

Changed 3 years ago by nthiery

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

Rename lattice() method space() in WeylGroups?. Add keys option to reflections()

comment:15 Changed 3 years ago by bump

Please also include #8414: in front of the patch description!

OK, I changed the patch description to begin #8414.

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
Note: See TracTickets for help on using tickets.