Opened 5 years ago
Closed 5 years ago
#18021 closed enhancement (fixed)
Add gyration orbit methods for AlternatingSignMatrix and AlternatingSignMatrices
Reported by:  egunawan  Owned by:  

Priority:  major  Milestone:  sage6.6 
Component:  combinatorics  Keywords:  days64, gyration, alternating sign matrix 
Cc:  jamespropp, tscrim, jessicapalencia, kdilks, vinceknight, jcampbell  Merged in:  
Authors:  Emily Gunawan, Travis Scrimshaw  Reviewers:  Jessica Striker, Travis Scrimshaw, James Propp 
Report Upstream:  N/A  Work issues:  
Branch:  b2478ae (Commits)  Commit:  b2478ae45ab4eec2a293812743f454412c776cd0 
Dependencies:  Stopgaps: 
Description (last modified by )
Pull this branch after checking out develop branch 6.6.rc0.
In class :class:AlternatingSignMatrix
(future: in class FullyPackedLoop?):
 add a method :meth:
obtain_gyration_orbit
for returning the orbit of self.
In class :class:AlternatingSignMatrices
:
 add a method :meth:
gyration_orbits
for returning the list of orbits of alternating sign matrices of order n.  add a method :meth:
gyration_orbit_size
for returning the sizes of orbits of alternating sign matrices of order n.
For method :meth:AlternatingSignMatrix.gyration
 optional parameter k (default k=1) to indicate how many times to gyrate on the ASM
(Note: we are no longer adding an optional parameter because the reviewer disagreed).
See Ticket #18003 (implementing FullyPackedLoop? class)
We are implementing the code that Travis Scrimshaw gave us.
Change History (23)
comment:1 Changed 5 years ago by
 Type changed from PLEASE CHANGE to enhancement
comment:2 Changed 5 years ago by
 Description modified (diff)
comment:3 Changed 5 years ago by
 Branch set to u/egunawan/ticket/18021
 Commit set to 52705536f95a497f1573665374c75b69f5d9037f
 Description modified (diff)
 Reviewers set to tscrim, jessicapalencia
 Status changed from new to needs_review
comment:4 Changed 5 years ago by
 Description modified (diff)
comment:5 Changed 5 years ago by
 Cc tscrim added
 Reviewers changed from tscrim, jessicapalencia to Jessica Striker, Travis Scrimshaw
comment:6 Changed 5 years ago by
 Cc jamespropp added
 Reviewers changed from Jessica Striker, Travis Scrimshaw to Jessica Striker, Travis Scrimshaw, James Propp
comment:7 Changed 5 years ago by
comment:8 followup: ↓ 11 Changed 5 years ago by
Here are my comments:
 I don't like the
count
option togyration
. You are much better writing a simplefor
loop which applies the sequence of gyrations to avoid the unnecessary complexity.  Rename
obtain_gyration_orbit
togyration_orbit
.  Use
@cached_method
forgyration_orbits
(and you should not say anything about its internal representation). Also have it return a tuple instead of a list.  Don't cache
gyration_orbit_sizes
since the previous caching will keep this fast (and definitely do not hardcode them).
Also, do you want this to depend on #17988? If so, I should be able to do that review this week.
comment:9 Changed 5 years ago by
This ticket does not depend on another ticket. Thanks Travis!
comment:10 Changed 5 years ago by
 Commit changed from 52705536f95a497f1573665374c75b69f5d9037f to 668f9ca72ff5ba6e3d7394459cbd49d44492d433
Branch pushed to git repo; I updated commit sha1. New commits:
668f9ca  #18021 Per Travis Scrimshaw's comments: remove count option for gyration method; rename obtain_gyration_orbit to gyration_orbit; use @cached_method for gyration_orbits; remove mention in doc about its internal representation; gyration_orbits now returns list of tuples (instead of list of lists); remove the (commented out) suggestion that we should hardcode gyration_orbit_sizes.

comment:11 in reply to: ↑ 8 Changed 5 years ago by
 Description modified (diff)
 Summary changed from gyration orbits method for AlternatingSignMatrix and add optional parameter for gyration method. to gyration orbit methods for AlternatingSignMatrix
I did all the changes you asked except for the very last comment "Don't cache gyration_orbit_sizes".
To clarify, do you mean I should remove these lines from gyration_orbit_sizes?
if self._gyration_orbit_sizes:
return self._gyration_orbit_sizes
And remove the "self._gyration_orbit_sizes"?
comment:12 Changed 5 years ago by
 Description modified (diff)
 Summary changed from gyration orbit methods for AlternatingSignMatrix to Add gyration orbit methods for AlternatingSignMatrix and AlternatingSignMatrices
comment:13 Changed 5 years ago by
Correct, computing len
of a list is very fast in python, and I doubt anyone will be repeatedly getting the sizes of the gyration orbits for ASM's with n = 100000
(it might actually be higher when you start seeing a noticeable effect).
Although my question in regards to #18003 (formerly #17988) is do you want this to or implement gyration on FPL's directly? Although it does make more sense to implement gyration in #18003.
comment:14 Changed 5 years ago by
We don't have any class called FullyPackedLoops? (plural) yet. The methods meth:gyration_orbits
and :meth:
gyration_orbit_sizes
requires a plural class like AlternatingSignMatrices?.
Also, I think Jessica would prefer that the ticket for the new FullyPackedLoop? class (#18003) be closed sooner rather than later. So I think it's better that we don't add any more methods to #18003.
comment:15 Changed 5 years ago by
 Commit changed from 668f9ca72ff5ba6e3d7394459cbd49d44492d433 to 2ba7a1386852ccb36b5fc375bcbe42f005cca8fd
Branch pushed to git repo; I updated commit sha1. New commits:
2ba7a13  #18021: Remove all instances of self._gyration_orbit_sizes

comment:16 Changed 5 years ago by
 Branch changed from u/egunawan/ticket/18021 to public/combinat/gyration18021
 Commit changed from 2ba7a1386852ccb36b5fc375bcbe42f005cca8fd to b2478ae45ab4eec2a293812743f454412c776cd0
Okay, I've made some review changes by cleaning up the code. In particular, you should try to not include any trailing whitespace in your code. Let me know if you have any questions.
New commits:
aefb4e7  Merge branch 'u/egunawan/ticket/18021' of trac.sagemath.org:sage into public/combinat/gyration18021

b2478ae  Some reviewer changes.

comment:17 Changed 5 years ago by
Also, if you're happy with my changes, then you can set this to a positive review.
comment:18 in reply to: ↑ description Changed 5 years ago by
 Description modified (diff)
comment:19 Changed 5 years ago by
Travis, I am happy with the changes. But I didn't check whether doc builds correctly. Is that OK?
comment:20 Changed 5 years ago by
You (or one of the other reviewers) should test it. You can do so by: sage docbuild reference/combinat html
(this will be the fastest way to rebuild just the part of the doc that changed) after running sage b
(if you haven't already).
comment:21 Changed 5 years ago by
 Status changed from needs_review to positive_review
Per emailing with Travis, I did make docclean and make doc, and the doc alternating_sign_matrix.html looks good to me.
comment:22 Changed 5 years ago by
Thanks all for your work on this!
comment:23 Changed 5 years ago by
 Branch changed from public/combinat/gyration18021 to b2478ae45ab4eec2a293812743f454412c776cd0
 Resolution set to fixed
 Status changed from positive_review to closed
Reviewers: Please search for [TODO] where I add comments/questions.
New commits:
Add methods gyration_orbits and gyration_orbit_sizes to class AlternatingSignMatrices
Add obtain_gyration_orbit method to AlternatingSignMatrix class
Add optional parameter count to method AlternatingSignMatrix.gyration