Opened 8 years ago
Closed 7 years ago
#18075 closed enhancement (fixed)
Add inversion number method to AlternatingSignMatrices
Reported by:  Jessica Striker  Owned by:  Jessica Striker 

Priority:  major  Milestone:  sage6.6 
Component:  combinatorics  Keywords:  asm, days64, aim 
Cc:  Travis Scrimshaw, Kevin Dilks, Emily Gunawan  Merged in:  
Authors:  Jessica Striker  Reviewers:  Darij Grinberg 
Report Upstream:  N/A  Work issues:  
Branch:  82c96d7 (Commits, GitHub, GitLab)  Commit:  82c96d70c29144b8523ea48fbc4d44fb06dda9ca 
Dependencies:  Stopgaps: 
Description
Add inversion number method to AlternatingSignMatrices
Change History (18)
comment:1 Changed 8 years ago by
comment:2 Changed 8 years ago by
Branch:  → u/jessicapalencia/inversion_number18075 

Commit:  → 8827ffe72e10cce5491849558a45f919440a4ff6 
Status:  new → needs_review 
New commits:
8827ffe  18075: added inversion number to AlternatingSignMatrices

comment:3 Changed 8 years ago by
Just a couple suggestions:
 Add the definition of the inversions number of an asm, typically with a reference
 Explain that for permutation matrices this matches with the usual inversion number of the permutation, with an example where this is tested for all permutations of, say, 5.
 Add a couple more examples
comment:5 Changed 8 years ago by
Status:  needs_review → needs_work 

Work issues:  → more tests and doc 
comment:6 Changed 8 years ago by
Commit:  8827ffe72e10cce5491849558a45f919440a4ff6 → 914e0ed6fbb1016c9b85300cc36dae0b2697492c 

comment:7 Changed 8 years ago by
Status:  needs_work → needs_review 

comment:8 Changed 8 years ago by
Branch:  u/jessicapalencia/inversion_number18075 → public/ticket/18075 

Commit:  914e0ed6fbb1016c9b85300cc36dae0b2697492c → a524fad6890beff2421b7d1acd1965aa21d42379 
comment:9 Changed 8 years ago by
Commit:  a524fad6890beff2421b7d1acd1965aa21d42379 → 0a442cb4befa9dbe0f5efa0df29d661dfbc7d788 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
0a442cb  ticket 18015 review patch

comment:10 Changed 8 years ago by
Work issues:  more tests and doc → error pointed out inside branch 

Ignore my previous commits, and use the one below. Thanks, and sorry.
New commits:
0a442cb  ticket 18015 review patch

comment:11 Changed 8 years ago by
Commit:  0a442cb4befa9dbe0f5efa0df29d661dfbc7d788 → 82c96d70c29144b8523ea48fbc4d44fb06dda9ca 

Branch pushed to git repo; I updated commit sha1. New commits:
82c96d7  correcting whitespace

comment:12 Changed 8 years ago by
Review branch uploaded.
A few remarks:
I am not 100% sure whether the letter ℓ goes against coding standards, but I have replaced it by a regular l to be safe. The docstrings are sent to latex, and I think latex doesn't like things that aren't ascii.
Also removed all the trailing whitespace.
The error in the iterator I pointed out should either be fixed here or moved to another ticket. Other than this, the ticket is good to go if you think the review changes are fine!
comment:13 Changed 8 years ago by
Reviewers:  → Darij Grinberg 

comment:14 Changed 7 years ago by
Hi Darij (or Travis),
Could you please open a new ticket for the error in the iterator or else fix the error on this ticket? I don't know exactly what the error is or how to fix it. Then can we set this ticket to positive review? Thanks!
comment:16 Changed 7 years ago by
Status:  needs_review → positive_review 

Yes, thanks for reviewing it!
comment:17 Changed 7 years ago by
Work issues:  error pointed out inside branch 

comment:18 Changed 7 years ago by
Branch:  public/ticket/18075 → 82c96d70c29144b8523ea48fbc4d44fb06dda9ca 

Resolution:  → fixed 
Status:  positive_review → closed 
first!