Opened 6 years ago

Closed 5 years ago

#20233 closed defect (fixed)

`AbstractLinearCode.minimum_distance` fails with GAP message for large fields

Reported by: jsrn Owned by:
Priority: major Milestone: sage-7.4
Component: coding theory Keywords: sd75
Cc: dlucas, djoyner Merged in:
Authors: Joe Fields Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 2809840 (Commits, GitHub, GitLab) Commit: 2809840d085219903103521bb9a79c74994b2005
Dependencies: Stopgaps:

Status badges

Description

The following works:

   C = LinearCode(random_matrix(GF(5^2,'a'), 2, 5)); C.minimum_distance()

while the following explodes with a GAP error

   C = LinearCode(random_matrix(GF(17^2,'a'), 2, 5)); C.minimum_distance()

It seems to be conditioned only on the size of the field: All fields with cardinality greater than 256 seem to fail.

Change History (11)

comment:1 Changed 5 years ago by jfields

  • Branch set to u/jfields/gap_min_dist_field_size

comment:2 Changed 5 years ago by jfields

  • Commit set to 2d4768234e1570307125c8479e77454871402705
  • Status changed from new to needs_review

The GAP algorithms which sage is calling for minimum_distance() have a (poorly) documented limitation -- field size of at most 256. I added a check on the field size and raise a "not implemented" exception in that case. So at least the user will get some explanation of the problem rather than an incomprehensible stack dump.


New commits:

2d47682Added a check on the size of the finite field in minimum_distance() computation.

comment:3 Changed 5 years ago by tscrim

Some minor things:

  • I would put the fact that this only works for fields up to 256 in either a .. NOTE:: or .. WARNING:: block so it's more prominent in the documentation.
  • You should add something just before the added doctest, something like The field must be size at most 256::. I would also split the line like this:
              NotImplementedError: The GAP algorithms that sage is using
               are limited to computing with fields of size at most 256.
    
  • To be PEP8 compliant, you should align the string start points:
              raise NotImplementedError("the GAP algorithm that Sage is using "
                                        "is limited to computing with fields "
                                        "of size at most 256")
    
    I've also suggested a few minor changes to the error message. Although I might consider limiting the actual message to something like the field must have size at most 256 with a more detailed comment just before it saying the issue is in GAP for those looking at the code itself.

comment:4 Changed 5 years ago by git

  • Commit changed from 2d4768234e1570307125c8479e77454871402705 to 4949d062c3fd912b3bb4fba2332b191e37f6968d

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

696edb0small fixes per tscrim's suggestions.
4949d06Fixed formatting of the ..warning:: block.

comment:5 Changed 5 years ago by git

  • Commit changed from 4949d062c3fd912b3bb4fba2332b191e37f6968d to dcaafe2e732cf40770f30a5ef0f2c0eac586b6ee

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

dcaafe2Added the sentence

comment:6 Changed 5 years ago by jfields

Not sure if I did what you intended regarding adding the sentence before the doctest.

comment:7 Changed 5 years ago by tscrim

  • Branch changed from u/jfields/gap_min_dist_field_size to u/tscrim/20233
  • Commit changed from dcaafe2e732cf40770f30a5ef0f2c0eac586b6ee to 2809840d085219903103521bb9a79c74994b2005
  • Milestone changed from sage-7.1 to sage-7.4
  • Reviewers set to Travis Scrimshaw

I made some small tweaks. If you're good with my changes, then you can set a positive review (and don't forget to add your real name to the authors field).


New commits:

72f01a7Merge branch 'u/jfields/gap_min_dist_field_size' of git://trac.sagemath.org/sage into u/jfields/gap_min_dist_field_size
2809840Some small reviewer changes.

comment:8 follow-up: Changed 5 years ago by jfields

  • Authors set to Joe Fields
  • Status changed from needs_review to positive_review

Thanks Travis, your changes are definitely fine. I've switched the status to "positive review." Please forgive a noob question: do I need to do anything git-wise about merging your changes? It looks like I don't but I'd rather be safe than sorry...

comment:9 Changed 5 years ago by jfields

  • Keywords sd75 added

Added the keyword sd75 to the ticket as this is work that began during the Sage Days 75 at INRIA Saclay.

comment:10 in reply to: ↑ 8 Changed 5 years ago by tscrim

Replying to jfields:

Thanks Travis, your changes are definitely fine. I've switched the status to "positive review." Please forgive a noob question: do I need to do anything git-wise about merging your changes? It looks like I don't but I'd rather be safe than sorry...

No, there's nothing you need to do (in fact, mine are based on a later version of Sage than your previous push, so you might not want to pull them at this point).

comment:11 Changed 5 years ago by vbraun

  • Branch changed from u/tscrim/20233 to 2809840d085219903103521bb9a79c74994b2005
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.