Opened 6 years ago
Closed 6 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: |
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 6 years ago by
- Branch set to u/jfields/gap_min_dist_field_size
comment:2 Changed 6 years ago by
- Commit set to 2d4768234e1570307125c8479e77454871402705
- Status changed from new to needs_review
comment:3 Changed 6 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 6 years ago by
- Commit changed from 2d4768234e1570307125c8479e77454871402705 to 4949d062c3fd912b3bb4fba2332b191e37f6968d
comment:5 Changed 6 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 6 years ago by
Not sure if I did what you intended regarding adding the sentence before the doctest.
comment:7 Changed 6 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 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:
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 follow-up: ↓ 10 Changed 6 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 git-wise about merging your changes? It looks like I don't but I'd rather be safe than sorry...
comment:9 Changed 6 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 6 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 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 6 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.