Opened 6 years ago

Closed 6 years ago

#15029 closed enhancement (fixed)

Implement similarity classes over principal ideal local rings of length two

Reported by: amri Owned by: amri
Priority: major Milestone: sage-5.13
Component: combinatorics Keywords: similarity classs, matrices, local rings
Cc: sage-combinat, tscrim Merged in: sage-5.13.beta0
Authors: Amritanshu Prasad Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #14907 Stopgaps:

Description (last modified by tscrim)

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:

Attachments (3)

trac_15029_similarity_local_rings-ap.patch (34.4 KB) - added by amri 6 years ago.
trac_15029-additions-similarity_class_type-ap.patch (30.4 KB) - added by amri 6 years ago.
Added functions for similarity over principal ideal local rings of length two
trac_15029-review-ts.patch (28.2 KB) - added by tscrim 6 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 6 years ago by amri

  • Cc sage-combinat tscrim added
  • Dependencies set to #14907
  • Reviewers set to tscrim
  • Status changed from new to needs_review

comment:2 Changed 6 years ago by tscrim

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

comment:3 Changed 6 years ago by tscrim

  • Reviewers changed from tscrim to Travis Scrimshaw
  • Status changed from needs_review to needs_work

Changed 6 years ago by amri

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

comment:4 Changed 6 years ago by amri

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

comment:5 Changed 6 years ago by amri

  • Status changed from needs_work to needs_review

Changed 6 years ago by tscrim

comment:6 Changed 6 years ago by tscrim

  • Description modified (diff)

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

comment:7 Changed 6 years ago by tscrim

For patchbot:

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

comment:8 Changed 6 years ago by amri

  • Status changed from needs_review to positive_review

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

Amri.

comment:9 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.12 to sage-5.13

comment:10 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.13.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.