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

Implement similarity classes over principal ideal local rings of length two #15029

Closed
amritanshu-prasad mannequin opened this issue Aug 10, 2013 · 12 comments
Closed

Implement similarity classes over principal ideal local rings of length two #15029

amritanshu-prasad mannequin opened this issue Aug 10, 2013 · 12 comments

Comments

@amritanshu-prasad
Copy link
Mannequin

amritanshu-prasad mannequin commented Aug 10, 2013

In the paper Similarity of matrices over local rings of length two http://arxiv.org/abs/1212.6157, the similarity classes of n x n matrices with entries in a principal ideal local ring of length two are computed for n = 2, 3, 4. These calculations are being implemented in sage.

Apply:

Depends on #14907

CC: @sagetrac-sage-combinat @tscrim

Component: combinatorics

Keywords: similarity classs, matrices, local rings

Author: Amritanshu Prasad

Reviewer: Travis Scrimshaw

Merged: sage-5.13.beta0

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

@amritanshu-prasad amritanshu-prasad mannequin added this to the sage-5.12 milestone Aug 10, 2013
@amritanshu-prasad amritanshu-prasad mannequin self-assigned this Aug 10, 2013
@amritanshu-prasad
Copy link
Mannequin Author

amritanshu-prasad mannequin commented Aug 10, 2013

@amritanshu-prasad
Copy link
Mannequin Author

amritanshu-prasad mannequin commented Aug 10, 2013

Dependencies: #14907

@amritanshu-prasad
Copy link
Mannequin Author

amritanshu-prasad mannequin commented Aug 10, 2013

Reviewer: tscrim

@tscrim
Copy link
Collaborator

tscrim commented Aug 15, 2013

comment:2

Hey Amri,

It seems like you've created 2 classes which are basically computing a few methods and don't contain helper methods. I would instead move these to (module-level) functions in similarity_class_type.py which take the data, q, selftranspose, invertible as arguments since there is:

  • no complicated internal state across multiple functions,
  • no complicated/expensive standardization or preparation needed on the inputs,
  • no element classes,
  • no classes inheriting from these.

Two other minor points:

  • Could you use the arXiv autolink format: :arxiv.org:`1212.6157`?
  • Could you remove the [mq]:... line from the patch header by doing a qrefresh -e?

Thanks,

Travis

@tscrim
Copy link
Collaborator

tscrim commented Aug 15, 2013

Changed reviewer from tscrim to Travis Scrimshaw

@amritanshu-prasad
Copy link
Mannequin Author

amritanshu-prasad mannequin commented Sep 9, 2013

Attachment: trac_15029-additions-similarity_class_type-ap.patch.gz

Added functions for similarity over principal ideal local rings of length two

@amritanshu-prasad
Copy link
Mannequin Author

amritanshu-prasad mannequin commented Sep 9, 2013

comment:4

As suggested, similarity classes for principal ideal local rings of length two is now implemented as functions in the module similarity_class_type.py

Sorry for all the whitespace changes; please ignore them.

Apply: trac_15029-additions-similarity_class_type-ap.patch

@tscrim

This comment has been minimized.

@tscrim
Copy link
Collaborator

tscrim commented Sep 9, 2013

comment:6

Attachment: trac_15029-review-ts.patch.gz

Hey Amri,

Here's a review patch which does a few things:

  • Changes the name of the functions *2 to *_length_two to be a little more explicit.
  • Adds INPUT: blocks to some of the functions.
  • Fixes some docstring mistakes I didn't catch the first time around in the whole file. I also converted it to use Sage's macro for finite fields \GF{q}.

If you're happy with my changes, go ahead and set this to positive review.

Thanks for your work on this,

Travis

@tscrim
Copy link
Collaborator

tscrim commented Sep 9, 2013

comment:7

For patchbot:

Apply: trac_15029-additions-similarity_class_type-ap.patch​, trac_15029-review-ts.patch​

@amritanshu-prasad
Copy link
Mannequin Author

amritanshu-prasad mannequin commented Sep 10, 2013

comment:8

Thanks Travis, for the considerable work. It looks great now.

Amri.

@jdemeyer
Copy link

jdemeyer commented Oct 2, 2013

Merged: sage-5.13.beta0

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

2 participants