Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Minimal fix for Algebraic Extensions over finite fields of order > 256. #1569

Merged
merged 2 commits into from
Aug 8, 2017
Merged

Minimal fix for Algebraic Extensions over finite fields of order > 256. #1569

merged 2 commits into from
Aug 8, 2017

Conversation

stevelinton
Copy link
Contributor

Code really needs rewriting, as does the whole issue of mechanisms to
construct/convert a vector in/to the most efficient form.

@stevelinton
Copy link
Contributor Author

Addresses #1561

@codecov
Copy link

codecov bot commented Aug 5, 2017 •
edited
Loading

Codecov Report

Merging #1569 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1569      +/-   ##
==========================================
+ Coverage   63.98%   63.98%   +<.01%     
==========================================
  Files         993      993              
  Lines      325155   325155              
  Branches    12995    12995              
==========================================
+ Hits       208040   208042       +2     
+ Misses     114324   114323       -1     
+ Partials     2791     2790       -1
Impacted Files Coverage Δ
lib/algfld.gi 24.64% <100%> (ø) ⬆️
src/hpc/traverse.c 78.29% <0%> (-0.39%) ⬇️
src/system.c 52.85% <0%> (-0.34%) ⬇️
src/stats.c 72.83% <0%> (-0.27%) ⬇️
src/hpc/threadapi.c 35.02% <0%> (-0.2%) ⬇️
src/stringobj.c 77.65% <0%> (-0.13%) ⬇️
lib/ffeconway.gi 82.5% <0%> (+0.11%) ⬆️
src/funcs.c 70.83% <0%> (+0.14%) ⬆️
src/vec8bit.c 72.08% <0%> (+0.15%) ⬆️
src/plist.c 88.26% <0%> (+0.15%) ⬆️
... and 2 more

@stevelinton
Copy link
Contributor Author

Very confused. The new test file runs fine when run using Test from the command-line, but fails when run as part of testbugfix.g or TestDirectory. Revised test file fixes various minor problems but does not seem to resolve this.

fingolfin
fingolfin reviewed Aug 7, 2017
View reviewed changes
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I run the bugfix test suite in my local GAP. It turns out that what fails is the command FieldByGenerators( GF(2), [ Z(1024) ] );. I'll track it down further.

tst/testbugfix/2017-08-05-algfld.tst Outdated
@@ -0,0 +1,14 @@
#
# Algebraic Extension used to fail for finite fields of size over 256
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: "Algebraic Extension" -> "AlgebraicExtension"

tst/testbugfix/2017-08-05-algfld.tst Outdated
#
# Algebraic Extension used to fail for finite fields of size over 256
#
gap> algexttest := function(q, i1,i2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpcik: Either remove space after one comma, or add one after the other

tst/testbugfix/2017-08-05-algfld.tst Outdated
> end;;
gap> algexttest(1009,108,864); # from bug report
gap> algexttest(1024,1023,5); # prime under 256, field size over
gap> algexttest(65537,1,1); # prime over 2^16
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: indention of this comment doesn't match the other comments

lib/algfld.gi
@@ -105,7 +105,7 @@ local fam,i,cof,red,rchar,impattr,deg;
fam!.deg:=deg;
i:=List([1..DegreeOfLaurentPolynomial(p)],i->fam!.zeroCoefficient);
i[2]:=fam!.oneCoefficient;
i:=ImmutableVector(rchar,i,true);
i:=ImmutableVector(Size(f),i,true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix looks correct to me.

Nitpick on the commits: The message "Add test for this bug." is not so helpful. I'd either squash the two commits into one, or amend the commit message, say to "Add test for bug in AlgebraicExtension"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also: If you add Fixes #1561 or Resolves #1561 to the commit message (say as its last line), then merging this PR will automatically close #1561.

@fingolfin fingolfin mentioned this pull request Aug 7, 2017
Merged
@stevelinton
Copy link
Contributor Author

I'll rebase and tidy once #1573 is merged

@fingolfin
Copy link
Member

@stevelinton #1573 was merged, you can rebase

@stevelinton
Minimal fix forAlgebraic Extensions over finite fields of order > 256.
3edf1cb 
Code really needs rewriting, as does the whole issue of mechanisms to
construct/convert a vector in/to the most efficient form.
Fixes gap-system#1561
@stevelinton
Add test for AlgebraicExtension over fields of size > 256
b443aff
@stevelinton
Copy link
Contributor Author

Rebased and addressed nitpicks. Ready to merge as far as I know.

fingolfin
fingolfin approved these changes Aug 8, 2017
View reviewed changes
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, let's see if tests pass now.

@fingolfin fingolfin merged commit 84e07ca into gap-system:master Aug 8, 2017
@fingolfin fingolfin added the kind: bug Issues describing general bugs, and PRs fixing them label Aug 8, 2017
@fingolfin fingolfin added this to the GAP 4.9.0 milestone Aug 8, 2017
@fingolfin fingolfin mentioned this pull request Sep 7, 2017
Closed
@olexandr-konovalov olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jan 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fingolfin fingolfin fingolfin approved these changes

Assignees
No one assigned
Labels
kind: bug Issues describing general bugs, and PRs fixing them release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Milestone
GAP 4.9.0
Development

Successfully merging this pull request may close these issues.
3 participants
@stevelinton @fingolfin @olexandr-konovalov