AbstractLinearCode.minimum_distance fails with GAP message for large fields

[johanrosenkilde]

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.

CC: @sagetrac-dlucas @wdjoyner

Component: coding theory

Keywords: sd75

Author: Joe Fields

Branch/Commit: 2809840

Reviewer: Travis Scrimshaw

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

[osj1961]

Branch: u/jfields/gap_min_dist_field_size

[osj1961]

comment:2

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:

2d47682

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

[osj1961]

Commit: 2d47682

[tscrim]

comment:3

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.

[sagetrac-git]

Changed commit from 2d47682 to 4949d06

[sagetrac-git]

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

696edb0	small fixes per tscrim's suggestions.
4949d06	Fixed formatting of the ..warning:: block.

[sagetrac-git]

Changed commit from 4949d06 to dcaafe2

[sagetrac-git]

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

dcaafe2

Added the sentence

[osj1961]

comment:6

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

[tscrim]

Reviewer: Travis Scrimshaw

[tscrim]

Changed branch from u/jfields/gap_min_dist_field_size to u/tscrim/20233

[tscrim]

comment:7

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:

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

[tscrim]

Changed commit from dcaafe2 to 2809840

[osj1961]

Author: Joe Fields

[osj1961]

comment:8

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...

[osj1961]

comment:9

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

[osj1961]

Changed keywords from none to sd75

[tscrim]

comment:10

Replying to @osj1961:

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).

[vbraun]

Changed branch from u/tscrim/20233 to 2809840