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.

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

Fix DoImmutableMatrix for finite fields (revised) #1504

Merged
merged 3 commits into from
Jul 17, 2017

Conversation

fingolfin
Copy link
Member

This PR replaces PR #1481. It contains all the changes of that, but also

frankluebeck and others added 3 commits July 14, 2017 20:43
DoImmutableMatrix wrongly assumed that finite fields of order q <= 256
are represented as GF(q) and applied the NC versions of ConvertToMatrixRep
to matrices over such fields which produced corrupted objects. E.g.:

gap> f1 := AlgebraicExtension(GF(3), CyclotomicPolynomial(GF(3), 5));;
gap> a := RootOfDefiningPolynomial(f1);;
gap> mat := [[a]];;
gap> DoImmutableMatrix(f1, mat, false);
< immutable compressed matrix 1x393286524 over GF(26) >
A previous commit added a check for IsFFECollection(field) (which
was suggested by me), but that is not actually correct, as "field"
can also be an integer. Instead, check IsFFECollColl(matrix).

Also fix a typo, change tabs to spaces, and refactor the code a
little bit.

And finally, add more systematic tests for ImmutableMatrix
@fingolfin fingolfin changed the title Mh/do immutable matrix Fix DoImmutableMatrix for finite fields (revised) Jul 14, 2017
@codecov
Copy link

codecov bot commented Jul 14, 2017

Codecov Report

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

@@            Coverage Diff             @@
##           master    #1504      +/-   ##
==========================================
+ Coverage   64.17%   64.18%   +<.01%     
==========================================
  Files         992      992              
  Lines      322141   322139       -2     
  Branches    13087    13092       +5     
==========================================
+ Hits       206749   206765      +16     
+ Misses     112570   112562       -8     
+ Partials     2822     2812      -10
Impacted Files Coverage Δ
lib/vecmat.gi 67.13% <100%> (-0.05%) ⬇️
hpcgap/lib/vecmat.gi 65.73% <100%> (-0.05%) ⬇️
src/hpc/threadapi.c 31.4% <0%> (-0.2%) ⬇️
lib/algfld.gi 24.64% <0%> (+0.08%) ⬆️
src/listfunc.c 77.12% <0%> (+0.17%) ⬆️
src/funcs.c 69.94% <0%> (+0.28%) ⬆️
src/hpc/traverse.c 79.77% <0%> (+0.38%) ⬆️
src/stats.c 72.93% <0%> (+0.39%) ⬆️
src/hpc/thread.c 47.03% <0%> (+0.79%) ⬆️
hpcgap/lib/upoly.gi 61.26% <0%> (+7.2%) ⬆️

@fingolfin fingolfin added the kind: bug Issues describing general bugs, and PRs fixing them label Jul 16, 2017
@markuspf markuspf merged commit c0eead1 into gap-system:master Jul 17, 2017
@fingolfin fingolfin deleted the mh/DoImmutableMatrix branch July 18, 2017 18:31
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.9.0 milestone Jan 20, 2018
@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
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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants