Opened 10 years ago

Closed 9 years ago

#13209 closed defect (fixed)

Fix some minor Cayley table documentation problems

Reported by: kcrisman Owned by: joyner
Priority: trivial Milestone: sage-5.9
Component: group theory Keywords: cayley doc table beginner
Cc: rbeezer, ryan Merged in: sage-5.9.beta0
Authors: Kannappan Sampath Reviewers: Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by kcrisman)

Things like letters should be 'letters', and I'm pretty sure that the list in elements should be a list of strings? This would be easy for people to check - just try out all options!

           INPUT:

            - ``names`` - the type of names used, values are:

              * ``letters`` - lowercase ASCII letters are used
                for a base 26 representation of the elements'
                positions in the list given by :meth:`list`,
                padded to a common width with leading 'a's.
              * ``digits`` - base 10 representation of the
                elements' positions in the list given by
                :meth:`~sage.matrix.operation_table.OperationTable.column_keys`,
                padded to a common width with leading zeros.
              * ``elements`` - the string representations
                of the elements themselves.
              * a list - a list of strings, where the length
                of the list equals the number of elements.

            - ``elements`` - default = ``None``.  A list of
              elements of the set.  This may be used to impose an
              alternate ordering on the elements, perhaps
              when this is used in the context of a particular structure.
              The default is to use whatever ordering is provided by the
              the group, which is reported by the 
              :meth:`~sage.matrix.operation_table.OperationTable.column_keys`
              method.  Or the ``elements`` can be a subset
              which is closed under the operation. In particular,
              this can be used when the base set is infinite.

Apply 17436.patch and 17437.3.patch.

Attachments (2)

17436.patch (12.1 KB) - added by knsam 9 years ago.
All Work relevant to this ticket has been carried out in this patch. No need to apply the previous patches... just this one will suffice.
17437.3.patch (6.0 KB) - added by knsam 9 years ago.
This recent patch addresses referees comments.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 10 years ago by knsam

I am unable to follow what this ticket is reporting. Since this is marked beginner, I would like to have a go at this one. Can someone please leave me a pointer or two at what is expected so I could try? Thank you. (Sorry if this is unconventional way of doing it.)

comment:2 follow-up: Changed 10 years ago by kcrisman

Thanks for trying, knsam! The issue is that ``letters`` should be ``'letters'`` and so should the others in that list be changed. Secondly, if you look at the thing that is actually done in the method for Cayley tables (a good beginner exercise is to search for where this code is!), you'll see that ``elements`` actually returns a list of strings, not the actual list of elements - I think!

Don't hesitate to ask more, of course. We like to make the curve as easy as possible to developing, though it could be even easier.

comment:3 in reply to: ↑ 2 ; follow-up: Changed 10 years ago by knsam

Replying to kcrisman: Thanks for your prompt reply!

Thanks for trying, knsam! The issue is that ``letters`` should be ``'letters'`` and so should the others in that list be changed. Secondly, if you look at the thing that is actually done in the method for Cayley tables (a good beginner exercise is to search for where this code is!), you'll see that ``elements`` actually returns a list of strings, not the actual list of elements - I think!

I have figured out where this piece of code is. But, I don't follow what you mean when you say elements returns a list of string as opposed to actual elements. Here's what I tried:

sage: G = CyclicPermutationGroup(4) 
sage: L = G.list()
[(), (1,2,3,4), (1,3)(2,4), (1,4,3,2)]
sage: L[2] * L[3] 
(1,2,3,4)

So, it does return a list of elements, not strings.

Don't hesitate to ask more, of course. We like to make the curve as easy as possible to developing, though it could be even easier.

Thanks for offerring to help.

Finally, you'd like that letters, elements be changed to 'letters' and 'elements', right?

comment:4 in reply to: ↑ 3 ; follow-up: Changed 10 years ago by kcrisman

So, it does return a list of elements, not strings.

Sure. But that's not what the keyword elements is referring to. Try using that keyword and see what happens if you pass elements and not strings.

Finally, you'd like that ``letters``, ``elements`` be changed to ``'letters'`` and ``'elements'``, right?

And ``digits`` as well. Not the elements referred to in the first half of this comment, though!

By the way, {{{stuff}}} is one way to mark up things that are code in these comments. The wiki:WikiFormatting link you should see also will have lots of other help for that.

comment:5 Changed 10 years ago by knsam

  • Reviewers set to kcrisman
  • Status changed from new to needs_review

comment:6 follow-up: Changed 10 years ago by kcrisman

  • Authors set to Kannappan Sampath
  • Reviewers changed from kcrisman to Karl-Dieter Crisman
  • Status changed from needs_review to needs_work

Sounds like you need to fix the "thingy", so "needs work".

This is weird.

sage: search_src('lowercase ASCII letters')
categories/additive_magmas.py:123:              * ``letters`` - lowercase ASCII letters are used
categories/additive_magmas.py:158:            elements as lowercase ASCII letters.  ::
categories/groups.py:113:              * ``letters`` - lowercase ASCII letters are used
categories/magmas.py:151:              * ``letters`` - lowercase ASCII letters are used
matrix/operation_table.py:39:      * ``letters`` - lowercase ASCII letters are used
matrix/operation_table.py:102:    The default symbol set for elements is lowercase ASCII letters,
matrix/operation_table.py:768:          * ``letters`` - lowercase ASCII letters are used

Apparently there are FIVE places where this is done? I guess it all uses OperationTable though. But these would all have to be fixed in the doc :(

comment:7 in reply to: ↑ 6 Changed 9 years ago by knsam

Replying to kcrisman:

Sounds like you need to fix the "thingy", so "needs work".

Yes, true! I'll try to get this fixed.

This is weird.

sage: search_src('lowercase ASCII letters')
categories/additive_magmas.py:123:              * ``letters`` - lowercase ASCII letters are used
categories/additive_magmas.py:158:            elements as lowercase ASCII letters.  ::
categories/groups.py:113:              * ``letters`` - lowercase ASCII letters are used
categories/magmas.py:151:              * ``letters`` - lowercase ASCII letters are used
matrix/operation_table.py:39:      * ``letters`` - lowercase ASCII letters are used
matrix/operation_table.py:102:    The default symbol set for elements is lowercase ASCII letters,
matrix/operation_table.py:768:          * ``letters`` - lowercase ASCII letters are used

Apparently there are FIVE places where this is done? I guess it all uses OperationTable though. But these would all have to be fixed in the doc :(

Here's what my Sage returns for this query:

matrix/operation_table.py:39:      * ``'letters'`` - lowercase ASCII letters are used
matrix/operation_table.py:102:    The default symbol set for elements is lowercase ASCII letters,
matrix/operation_table.py:768:          * ``'letters'`` - lowercase ASCII letters are used
categories/groups.py:113:              * ``letters`` - lowercase ASCII letters are used
categories/magmas.py:151:              * ``letters`` - lowercase ASCII letters are used
categories/additive_magmas.py:123:              * ``letters`` - lowercase ASCII letters are used
categories/additive_magmas.py:158:            elements as lowercase ASCII letters.  ::

Clearly, the first and third have been fixed. And, the second is of irrelevant nature. Similarly, the last one. And, the other two required fix. I have fixed it.

comment:8 in reply to: ↑ 4 Changed 9 years ago by knsam

Replying to kcrisman:

So, it does return a list of elements, not strings.

Sure. But that's not what the keyword elements is referring to. Try using that keyword and see what happens if you pass elements and not strings.

Alright, I think there is no real issue with this one. Well, what I mean is, since (foo, bar) stands for a tuple, having things like (1,2)(3,4) scares Sage. Reasonable. I looked at how its overcome in general:

sage: G = SymmetricGroup(5)
sage: sigma = G("(1,3) (2,5,4)")

which is basically coercion at play. So, there are two work arounds:

  1. Pass the elements as strings, because after all the code coerces them into an element of the said group. *(done in the documentation)*
  2. Or, suppose you have a group G. List its elements using G.list(). Now determine the indices which you'd like to include in the list and use a for loop to iterate.

Here's a test case:

sage: H = CyclicPermutationGroup(4)
sage: L = H.list()
sage: elts = [] 
sage: for i in {0, 2}: 
...      elts.append(L[i])
...
sage: elts
[(), (1,3) (2,4)] 
sage: from sage.matrix.operation_table import OperationTable
sage: T = OperationTable(H, operator.mul, elements = elts) 
sage: T
*  a b
 +----
a| a b
b| b a

So, does this solve the "thingy" issue? I will submit a patch for the other two OperationTable? docs. And, should this alternative way of passing elements be documented?

comment:9 Changed 9 years ago by kcrisman

I think you didn't attach the right patch; this one doesn't have any changes to the categories directory.

I see what is going on. So you are saying that one can pass elements either as a list of elements OR as a list of strings? I guess I must have never actually tried doing it as elements! Yes, in that case every relevant case should probably have a (brief) example demonstrating that you can pass strings as well, and the documentation should say that it can be a list of strings or a list of elements.

Changed 9 years ago by knsam

All Work relevant to this ticket has been carried out in this patch. No need to apply the previous patches... just this one will suffice.

comment:10 Changed 9 years ago by knsam

  • Status changed from needs_work to needs_review

comment:11 follow-up: Changed 9 years ago by kcrisman

Looking better! Thanks for working with the often-multistep process.

A couple things:

  • Very minor - you may want to name the patch after the ticket issue, like trac_13209-cayley.patch or something. No biggie.
  • Slightly less minor - the commit message is very long. Try to have a short one first, then in the next lines more detail. I think the limit is 88 characters or whatever the standard terminal window is. I thought this was in the developer guide but I can't find it...
  • Same issue with the documentation changes. They look good, but you made some lines too long. Try to keep them the same length as the current lines. This is a big enough deal to make it "needs work".
  • Also, although you are probably right about the "able to be coerced" thing, it would be nice to make it explicit that strings work. That is why I originally opened the ticket. Maybe "such as strings" or something. I know this is just my personal preference, but I think it's likely that it's a use case; people may not be able to easily get the exact elements they want for passing in in a given order (if it's a strange order) but it's easy to type those elements in, in which case they will need to be strings, like in one of the examples.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 9 years ago by knsam

Replying to kcrisman:

Looking better! Thanks for working with the often-multistep process.

Thank you! I'll take care of the commit message and the renaming the patch.

A couple things:

  • Same issue with the documentation changes. They look good, but you made some lines too long. Try to keep them the same length as the current lines. This is a big enough deal to make it "needs work".

Did you mean to say that the language is verbose or the 88 character-bound is being broken too often?

  • Also, although you are probably right about the "able to be coerced" thing, it would be nice to make it explicit that strings work. That is why I originally opened the ticket. Maybe "such as strings" or something. I know this is just my personal preference, but I think it's likely that it's a use case; people may not be able to easily get the exact elements they want for passing in in a given order (if it's a strange order) but it's easy to type those elements in, in which case they will need to be strings, like in one of the examples.

Noted! I will do this one now.

comment:13 in reply to: ↑ 12 Changed 9 years ago by kcrisman

  • Same issue with the documentation changes. They look good, but you made some lines too long. Try to keep them the same length as the current lines. This is a big enough deal to make it "needs work".

Did you mean to say that the language is verbose or the 88 character-bound is being broken too often?

The latter. In principle, it shouldn't be broken at all, though there are exceptions. I hope it actually is 88 characters - maybe 80 characters? That's why I just make it no longer than existing documentation!

Changed 9 years ago by knsam

This recent patch addresses referees comments.

comment:14 follow-up: Changed 9 years ago by kcrisman

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

Okay, patchbot says yes, and although this isn't how I would have written it I think it is definitely better than before - we each have our own style, n'est pas?. Thanks!

Also, I think I have the correct patches selected - let me know if the description needs to be changed for which patches are to be merged!

comment:15 in reply to: ↑ 14 Changed 9 years ago by knsam

Replying to kcrisman:

Okay, patchbot says yes, and although this isn't how I would have written it I think it is definitely better than before - we each have our own style, n'est pas?. Thanks!

Also, I think I have the correct patches selected - let me know if the description needs to be changed for which patches are to be merged!

Hello! The patches to be merged are indicated correctly. Hmm, I'd also be glad to know if I should change the style of my writing - your comments are always welcome. As you know, I am new to this and would like to be corrected.

comment:16 Changed 9 years ago by kcrisman

Style is just that - style. If there was something mathematically wrong (that I noticed) or different from our guidelines I'd say so (and did above); no worries.

comment:17 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-5.8 to sage-5.9

comment:18 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.9.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.