Opened 4 years ago

Closed 4 years ago

#18021 closed enhancement (fixed)

Add gyration orbit methods for AlternatingSignMatrix and AlternatingSignMatrices

Reported by: egunawan Owned by:
Priority: major Milestone: sage-6.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 egunawan)

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 4 years ago by egunawan

  • Type changed from PLEASE CHANGE to enhancement

comment:2 Changed 4 years ago by egunawan

  • Description modified (diff)

comment:3 Changed 4 years ago by egunawan

  • 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

Reviewers: Please search for [TODO] where I add comments/questions.


New commits:

be50f3fAdd methods gyration_orbits and gyration_orbit_sizes to class AlternatingSignMatrices
26264f5Add obtain_gyration_orbit method to AlternatingSignMatrix class
5270553Add optional parameter count to method AlternatingSignMatrix.gyration

comment:4 Changed 4 years ago by egunawan

  • Description modified (diff)

comment:5 Changed 4 years ago by egunawan

  • Cc tscrim added
  • Reviewers changed from tscrim, jessicapalencia to Jessica Striker, Travis Scrimshaw

comment:6 Changed 4 years ago by egunawan

  • Cc jamespropp added
  • Reviewers changed from Jessica Striker, Travis Scrimshaw to Jessica Striker, Travis Scrimshaw, James Propp

comment:7 Changed 4 years ago by egunawan

  • Authors set to Emily Gunawan, Travis Scrimshaw

comment:8 follow-up: Changed 4 years ago by tscrim

Here are my comments:

  • I don't like the count option to gyration. You are much better writing a simple for loop which applies the sequence of gyrations to avoid the unnecessary complexity.
  • Rename obtain_gyration_orbit to gyration_orbit.
  • Use @cached_method for gyration_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 4 years ago by egunawan

This ticket does not depend on another ticket. Thanks Travis!

comment:10 Changed 4 years ago by git

  • 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 4 years ago by egunawan

  • 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 4 years ago by egunawan

  • Description modified (diff)
  • Summary changed from gyration orbit methods for AlternatingSignMatrix to Add gyration orbit methods for AlternatingSignMatrix and AlternatingSignMatrices

comment:13 Changed 4 years ago by tscrim

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 4 years ago by egunawan

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.

Last edited 4 years ago by egunawan (previous) (diff)

comment:15 Changed 4 years ago by git

  • 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 4 years ago by tscrim

  • Branch changed from u/egunawan/ticket/18021 to public/combinat/gyration-18021
  • 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:

aefb4e7Merge branch 'u/egunawan/ticket/18021' of trac.sagemath.org:sage into public/combinat/gyration-18021
b2478aeSome reviewer changes.

comment:17 Changed 4 years ago by tscrim

Also, if you're happy with my changes, then you can set this to a positive review.

comment:18 in reply to: ↑ description Changed 4 years ago by egunawan

  • Description modified (diff)
Last edited 4 years ago by egunawan (previous) (diff)

comment:19 Changed 4 years ago by egunawan

Travis, I am happy with the changes. But I didn't check whether doc builds correctly. Is that OK?

comment:20 Changed 4 years ago by tscrim

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).

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

comment:21 Changed 4 years ago by egunawan

  • Status changed from needs_review to positive_review

Per emailing with Travis, I did make doc-clean and make doc, and the doc alternating_sign_matrix.html looks good to me.

comment:22 Changed 4 years ago by tscrim

Thanks all for your work on this!

comment:23 Changed 4 years ago by vbraun

  • Branch changed from public/combinat/gyration-18021 to b2478ae45ab4eec2a293812743f454412c776cd0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.