Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add inversion number method to AlternatingSignMatrices #18075

Closed
jessicapalencia mannequin opened this issue Mar 27, 2015 · 25 comments
Closed

Add inversion number method to AlternatingSignMatrices #18075

jessicapalencia mannequin opened this issue Mar 27, 2015 · 25 comments

Comments

@jessicapalencia
Copy link
Mannequin

jessicapalencia mannequin commented Mar 27, 2015

Add inversion number method to AlternatingSignMatrices

CC: @tscrim @kevindilks @egunawan

Component: combinatorics

Keywords: asm, days64, aim

Author: Jessica Striker

Branch/Commit: 82c96d7

Reviewer: Darij Grinberg

Issue created by migration from https://trac.sagemath.org/ticket/18075

@jessicapalencia jessicapalencia mannequin added this to the sage-6.6 milestone Mar 27, 2015
@jessicapalencia jessicapalencia mannequin self-assigned this Mar 27, 2015
@darijgr
Copy link
Contributor

darijgr commented Mar 27, 2015

comment:1

first!

@jessicapalencia
Copy link
Mannequin Author

jessicapalencia mannequin commented Mar 27, 2015

New commits:

8827ffe18075: added inversion number to AlternatingSignMatrices

@jessicapalencia
Copy link
Mannequin Author

jessicapalencia mannequin commented Mar 27, 2015

Commit: 8827ffe

@jessicapalencia
Copy link
Mannequin Author

jessicapalencia mannequin commented Mar 27, 2015

@nthiery
Copy link
Contributor

nthiery commented Mar 27, 2015

comment:3

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

@nthiery
Copy link
Contributor

nthiery commented Mar 27, 2015

comment:4

Thanks for your contribution to Sage :-)

@nthiery
Copy link
Contributor

nthiery commented Mar 27, 2015

Work Issues: more tests and doc

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 30, 2015

Branch pushed to git repo; I updated commit sha1. New commits:

590afd1documentation and example added
914e0edAdded another test

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 30, 2015

Changed commit from 8827ffe to 914e0ed

@darijgr
Copy link
Contributor

darijgr commented Mar 30, 2015

New commits:

018f406Merge branch 'u/jessicapalencia/inversion_number-18075' of git://trac.sagemath.org/sage into asm
c6d341a#18075 review patch
a524fadticket 18075 review patch

@darijgr
Copy link
Contributor

darijgr commented Mar 30, 2015

Changed commit from 914e0ed to a524fad

@darijgr
Copy link
Contributor

darijgr commented Mar 30, 2015

Changed branch from u/jessicapalencia/inversion_number-18075 to public/ticket/18075

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 30, 2015

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

0a442cbticket 18015 review patch

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 30, 2015

Changed commit from a524fad to 0a442cb

@darijgr
Copy link
Contributor

darijgr commented Mar 30, 2015

Changed work issues from more tests and doc to error pointed out inside branch

@darijgr
Copy link
Contributor

darijgr commented Mar 30, 2015

comment:10

Ignore my previous commits, and use the one below. Thanks, and sorry.


New commits:

0a442cbticket 18015 review patch

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 30, 2015

Branch pushed to git repo; I updated commit sha1. New commits:

82c96d7correcting whitespace

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 30, 2015

Changed commit from 0a442cb to 82c96d7

@darijgr
Copy link
Contributor

darijgr commented Mar 30, 2015

comment:12

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!

@darijgr
Copy link
Contributor

darijgr commented Mar 30, 2015

Reviewer: Darij Grinberg

@jessicapalencia
Copy link
Mannequin Author

jessicapalencia mannequin commented Apr 15, 2015

comment:14

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!

@darijgr
Copy link
Contributor

darijgr commented Apr 15, 2015

comment:15

Done, #18208. Can you pos_rev this one then?

@jessicapalencia
Copy link
Mannequin Author

jessicapalencia mannequin commented Apr 15, 2015

comment:16

Yes, thanks for reviewing it!

@tscrim
Copy link
Collaborator

tscrim commented Apr 15, 2015

Changed work issues from error pointed out inside branch to none

@vbraun
Copy link
Member

vbraun commented Apr 15, 2015

Changed branch from public/ticket/18075 to 82c96d7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants