Opened 5 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:  sage7.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: 
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
 Branch set to u/jfields/gap_min_dist_field_size
comment:2 Changed 5 years ago by
 Commit set to 2d4768234e1570307125c8479e77454871402705
 Status changed from new to needs_review
comment:3 Changed 5 years ago by
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:
I've also suggested a few minor changes to the error message. Although I might consider limiting the actual message to something like
raise NotImplementedError("the GAP algorithm that Sage is using " "is limited to computing with fields " "of size at most 256")
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
 Commit changed from 2d4768234e1570307125c8479e77454871402705 to 4949d062c3fd912b3bb4fba2332b191e37f6968d
comment:5 Changed 5 years ago by
 Commit changed from 4949d062c3fd912b3bb4fba2332b191e37f6968d to dcaafe2e732cf40770f30a5ef0f2c0eac586b6ee
Branch pushed to git repo; I updated commit sha1. New commits:
dcaafe2  Added the sentence

comment:6 Changed 5 years ago by
Not sure if I did what you intended regarding adding the sentence before the doctest.
comment:7 Changed 5 years ago by
 Branch changed from u/jfields/gap_min_dist_field_size to u/tscrim/20233
 Commit changed from dcaafe2e732cf40770f30a5ef0f2c0eac586b6ee to 2809840d085219903103521bb9a79c74994b2005
 Milestone changed from sage7.1 to sage7.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:
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.

comment:8 followup: ↓ 10 Changed 5 years ago by
 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 gitwise 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
 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
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 gitwise 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
 Branch changed from u/tscrim/20233 to 2809840d085219903103521bb9a79c74994b2005
 Resolution set to fixed
 Status changed from positive_review to closed
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:
Added a check on the size of the finite field in minimum_distance() computation.