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

Support consistency checks for Vector, Matrix #5153

Merged
merged 3 commits into from
Nov 6, 2022
Merged

Support consistency checks for Vector, Matrix #5153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6704749
support consistency checks for `Vector`, `Matrix`
ThomasBreuer Oct 20, 2022
031cdfb
more documentation, more consistency checks
ThomasBreuer Nov 2, 2022
869a60b
adjusted a test to the new consistency checks
ThomasBreuer Nov 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 33 additions & 4 deletions doc/ref/matobj.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,9 @@ for the new type of vector or matrix objects.
<Ref Oper="\[\]" Label="for a vector object and an integer"/>,
</Item>
<Item>
<Ref Oper="\[\]\:\=" Label="for a vector object and an integer"/>,
<Ref Oper="\[\]\:\=" Label="for a vector object and an integer"/>
(with consistency checks if the global option <C>check</C> is not set to
<K>false</K>),
</Item>
<Item>
<Ref Oper="\&lt;"/> (see <Ref Oper="\&lt;" Label="for two vector objects"/>),
Expand All @@ -246,7 +248,9 @@ for the new type of vector or matrix objects.
<Ref Attr="ConstructingFilter" Label="for a vector object"/>,
</Item>
<Item>
<Ref Constr="NewVector"/>.
<Ref Constr="NewVector"/>
(with consistency checks if the global option <C>check</C> is not set to
<K>false</K>).
</Item>
</List>

Expand All @@ -268,7 +272,9 @@ for the new type of vector or matrix objects.
<Ref Oper="MatElm"/>,
</Item>
<Item>
<Ref Oper="SetMatElm"/>,
<Ref Oper="SetMatElm"/>
(with consistency checks if the global option <C>check</C> is not set to
<K>false</K>),
</Item>
<Item>
<Ref Oper="\&lt;"/> (see <Ref Oper="\&lt;" Label="for two matrix objects"/>),
Expand All @@ -280,10 +286,33 @@ for the new type of vector or matrix objects.
<Ref Attr="CompatibleVectorFilter" Label="for a matrix object"/>,
</Item>
<Item>
<Ref Constr="NewMatrix"/>.
<Ref Constr="NewMatrix"/>
(with consistency checks if the global option <C>check</C> is not set to
<K>false</K>).
</Item>
</List>

<P/>

Methods for the constructors <Ref Constr="NewVector"/> and
<Ref Constr="NewMatrix"/> must check their arguments for consistency
(do the given filter and base domain fit together,
are the entries compatible with the given base domain,
is the number of matrix entries a multiple of the given number of columns)
except if the global option <C>check</C> is set to <K>false</K>.
(See Chapter <Ref Chap="Options Stack"/> for information about global
options.)
The same holds for methods for operations that modify mutable vector
or matrix objects,
such as <Ref Oper="\[\]\:\=" Label="for a vector object and an integer"/>,
<Ref Oper="SetMatElm"/>, <Ref Oper="CopySubVector"/>,
<Ref Oper="CopySubMatrix"/>,
and for those methods of
<Ref Oper="Vector" Label="for filter, base domain, and list"/> and
<Ref Oper="Matrix" Label="for filter, base domain, list, ncols"/>
that do not delegate to <Ref Constr="NewVector"/> and
<Ref Constr="NewMatrix"/>, respectively.

</Section>

<!-- %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%% -->
Expand Down
14 changes: 14 additions & 0 deletions lib/matobj.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,17 @@ end);

# methods to create vector objects

#############################################################################
##
#O Vector( <filt>, <R>, <list> )
#O Vector( <filt>, <R>, <vec> )
#O Vector( <R>, <list> )
#O Vector( <R>, <vec> )
#O Vector( <list>, <vec> )
#O Vector( <vec1>, <vec2> )
##
## Compute the missing arguments for 'NewVector' and then call it.
##
InstallMethod( Vector,
[ IsOperation, IsSemiring, IsList ],
NewVector );
Expand Down Expand Up @@ -283,6 +294,8 @@ InstallMethod( ZeroVector,
#M Matrix( <list>, <ncols> )
#M Matrix( <list> )
##
## Compute the missing arguments for 'NewMatrix' and then call it.
##
InstallMethod( Matrix,
[ IsOperation, IsSemiring, IsList, IsInt ],
{ filt, R, list, nrCols } -> NewMatrix( filt, R, nrCols, list ) );
Copy link
Member

Choose a reason for hiding this comment

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

Wasn't the idea to insert (optional) checks into these Matrix functions, to validate things like that nrCols and Length(list) are compatible, that the given lists don't have holes, that entries are contained in the given basedomain etc ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My point of view is as follows:

The documentation says that implementing new types of matrix objects requires the installation of a NewMatrix method,
whereas there are default Matrix methods which compute the missing arguments for NewMatrix.
The question whether the arguments of Matrix (and hence the arguments that are then passed to NewMatrix) are consistent depends on the type of the matrix objects in question; it may be required that all matrix entries lie in the base domain, or more general situations are possible. The documentation is (deliberately?) vague in this respect, and adding the concrete consistency conditions to the NewMatrix method in question will make precise what is allowed for the given type of matrix objects.

I think that consistency checks in Matrix methods can be either only partial checks (which need more checks in the NewMatrix methods) or default checks (which need individual Matrix methods with modified checks if the default does not fit). Wouldn't that be more complicated in the end?

Expand Down Expand Up @@ -390,6 +403,7 @@ InstallMethod( Matrix,
return NewMatrix( ConstructingFilter(example), BaseDomain(example), NrCols(mat), Unpack(mat) );
end );


#
#
#
Expand Down
32 changes: 27 additions & 5 deletions lib/matobj2.gd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,10 +266,9 @@ DeclareAttribute( "CompatibleVectorFilter", IsMatrixOrMatrixObj );
## that contains the <A>list</A><M>[ k ]</M>-th entry of <A>v</A> at
## position <M>k</M>.
## <P/>
## It is not specified what happens if <A>i</A> is larger than the length
## of <A>v</A>,
## or if <A>obj</A> is not in the base domain of <A>v</A>,
## or if <A>list</A> contains entries not in the allowed range.
## If the global option <C>check</C> is set to <K>false</K> then
## <Ref Oper="\[\]\:\=" Label="for a vector object and an integer"/>
## need not perform consistency checks.
## <P/>
## Note that the sublist assignment operation <Ref Oper="\{\}\:\="/>
## is left out here since it tempts the programmer to use constructions like
Expand Down Expand Up @@ -646,6 +645,10 @@ DeclareOperation( "ZeroVector", [ IsInt, IsVecOrMatObj ] );
## this list.
## <P/>
## It is <E>not</E> guaranteed that the given list of entries is copied.
## <P/>
## If the global option <C>check</C> is set to <K>false</K> then
## <Ref Oper="Vector" Label="for filter, base domain, and list"/>
## need not perform consistency checks.
## </Description>
## </ManSection>
## <#/GAPDoc>
Expand Down Expand Up @@ -680,6 +683,9 @@ DeclareOperation( "Vector", [ IsList ] );
## and the entries in <A>list</A>.
## The list <A>list</A> is guaranteed not to be changed by this operation.
## <P/>
## If the global option <C>check</C> is set to <K>false</K> then
## <Ref Oper="NewVector"/> need not perform consistency checks.
## <P/>
## Similarly, <Ref Constr="NewZeroVector"/> returns a mutable vector object
## of length <A>n</A> which has <A>filt</A> and <A>R</A> as
## <Ref Attr="ConstructingFilter" Label="for a vector object"/> and
Expand Down Expand Up @@ -726,6 +732,9 @@ DeclareConstructor( "NewZeroVector", [ IsVectorObj, IsSemiring, IsInt ] );
## The corresponding entries must be in or compatible with <A>R</A>.
## If <A>list</A> already contains vector objects, they are copied.
## <P/>
## If the global option <C>check</C> is set to <K>false</K> then
## <Ref Oper="NewMatrix"/> need not perform consistency checks.
## <P/>
## Similarly, <Ref Constr="NewZeroMatrix"/> returns a mutable zero matrix
## object with <A>m</A> rows and <A>n</A> columns
## which has <A>filt</A> and <A>R</A> as
Expand Down Expand Up @@ -843,6 +852,9 @@ DeclareOperation( "Randomize", [ IsRandomSource, IsMatrixOrMatrixObj and IsMutab
## For certain objects like compressed vectors this might be significantly
## more efficient if <A>scols</A> and <A>dcols</A> are ranges
## with increment 1.
## <P/>
## If the global option <C>check</C> is set to <K>false</K> then
## <Ref Oper="CopySubVector"/> need not perform consistency checks.
## </Description>
## </ManSection>
## <#/GAPDoc>
Expand Down Expand Up @@ -945,6 +957,9 @@ DeclareOperation( "MutableCopyMatrix", [ IsMatrixOrMatrixObj ] );
## For certain objects like compressed vectors this might be
## significantly more efficient if <A>scols</A> and <A>dcols</A> are
## ranges with increment 1.
## <P/>
## If the global option <C>check</C> is set to <K>false</K> then
## <Ref Oper="CopySubMatrix"/> need not perform consistency checks.
## </Description>
## </ManSection>
## <#/GAPDoc>
Expand Down Expand Up @@ -1008,6 +1023,9 @@ DeclareSynonym( "MatElm", ELM_MAT );
## <A>M</A><C>[ </C><A>row</A><C> ][ </C><A>col</A><C> ]:= </C><A>obj</A>,
## which would first try to access <A>M</A><C>[ </C><A>row</A><C> ]</C>,
## and this is in general not possible.
## <P/>
## If the global option <C>check</C> is set to <K>false</K> then
## <Ref Oper="SetMatElm"/> need not perform consistency checks.
## </Description>
## </ManSection>
## <#/GAPDoc>
Expand Down Expand Up @@ -1194,7 +1212,7 @@ DeclareOperation( "CompanionMatrix",
## value <A>filt</A>, is defined over the base domain <A>R</A>,
## and has the entries given by the list <A>list</A> or the matrix object
## <A>M</A>, respectively.
## Here <A>list</A> can be either a list of plain list that describe the
## Here <A>list</A> can be either a list of plain lists that describe the
## entries of the rows, or a flat list of the entries in row major order,
## where <A>ncols</A> defines the number of columns.
## <P/>
Expand Down Expand Up @@ -1222,6 +1240,10 @@ DeclareOperation( "CompanionMatrix",
## of <Ref Oper="ShallowCopy"/>.
## If <A>list</A> is a nested list then it is <E>not</E> guaranteed
## that also the entries of <A>list</A> are copied.
## <P/>
## If the global option <C>check</C> is set to <K>false</K> then
## <Ref Oper="Matrix" Label="for filter, base domain, list, ncols"/>
## need not perform consistency checks.
## </Description>
## </ManSection>
## <#/GAPDoc>
Expand Down
43 changes: 39 additions & 4 deletions lib/matobjnz.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,19 @@ InstallMethod( CompatibleVectorFilter, "zmodnz",
InstallMethod( NewVector, "for IsZmodnZVectorRep, a ring, and a list",
[ IsZmodnZVectorRep, IsRing, IsList ],
function( filter, basedomain, l )
local typ, v;
local check, typ, v;
check:= ValueOption( "check" ) <> false;
if check and not ( IsZmodnZObjNonprimeCollection( basedomain ) or
( IsFinite( basedomain ) and IsPrimeField( basedomain ) ) ) then
Error( "<basedomain> must be Integers mod <n> for some <n>" );
fi;
typ:=NewType(FamilyObj(basedomain),IsZmodnZVectorRep and IsMutable and
CanEasilyCompareElements);
# force list of integers
if FamilyObj(basedomain)=FamilyObj(l) then
l:=List(l,Int);
elif check and not ForAll( l, IsInt ) then
Error( "<l> must be a list of integers or of elements in <basedomain>" );
else
l:=ShallowCopy(l);
fi;
Expand All @@ -59,7 +66,12 @@ InstallMethod( NewVector, "for IsZmodnZVectorRep, a ring, and a list",
InstallMethod( NewZeroVector, "for IsZmodnZVectorRep, a ring, and an int",
[ IsZmodnZVectorRep, IsRing, IsInt ],
function( filter, basedomain, l )
local typ, v;
local check, typ, v;
check:= ValueOption( "check" ) <> false;
if check and not ( IsZmodnZObjNonprimeCollection( basedomain ) or
( IsFinite( basedomain ) and IsPrimeField( basedomain ) ) ) then
Error( "<basedomain> must be Integers mod <n> for some <n>" );
fi;
typ:=NewType(FamilyObj(basedomain),IsZmodnZVectorRep and IsMutable and
CanEasilyCompareElements);
# represent list as integers
Expand Down Expand Up @@ -582,6 +594,9 @@ InstallMethod( CopySubVector, "for two zmodnz vectors and two lists",
[ IsZmodnZVectorRep, IsZmodnZVectorRep and IsMutable, IsList, IsList ],
function( a,b,pa,pb )
# The following should eventually go into the kernel:
if ValueOption( "check" ) <> false and a![BDPOS] <> b![BDPOS] then
Error( "<a> and <b> have different base domains" );
fi;
b![ELSPOS]{pb} := a![ELSPOS]{pa};
end );

Expand All @@ -600,7 +615,13 @@ InstallMethod( NewMatrix,
"for IsZmodnZMatrixRep, a ring, an int, and a list",
[ IsZmodnZMatrixRep, IsRing, IsInt, IsList ],
function( filter, basedomain, rl, l )
local nd, filterVectors, m, e, filter2, i;
local check, nd, filterVectors, m, e, filter2, i;

check:= ValueOption( "check" ) <> false;
if check and not ( IsZmodnZObjNonprimeCollection( basedomain ) or
( IsFinite( basedomain ) and IsPrimeField( basedomain ) ) ) then
Error( "<basedomain> must be Integers mod <n> for some <n>" );
fi;

# If applicable then replace a flat list 'l' by a nested list
# of lists of length 'rl'.
Expand Down Expand Up @@ -639,7 +660,14 @@ InstallMethod( NewZeroMatrix,
"for IsZmodnZMatrixRep, a ring, and two ints",
[ IsZmodnZMatrixRep, IsRing, IsInt, IsInt ],
function( filter, basedomain, rows, cols )
local m,i,e,filter2;
local check, m,i,e,filter2;

check:= ValueOption( "check" ) <> false;
if check and not ( IsZmodnZObjNonprimeCollection( basedomain ) or
( IsFinite( basedomain ) and IsPrimeField( basedomain ) ) ) then
Error( "<basedomain> must be Integers mod <n> for some <n>" );
fi;

filter2 := IsZmodnZVectorRep;
m := 0*[1..rows];
e := NewVector(filter2, basedomain, []);
Expand Down Expand Up @@ -974,6 +1002,9 @@ InstallMethod( CopySubMatrix, "for two zmodnz matrices and four lists",
IsList, IsList, IsList, IsList ],
function( m, n, srcrows, dstrows, srccols, dstcols )
local i;
if ValueOption( "check" ) <> false and m![BDPOS] <> n![BDPOS] then
Error( "<m> and <n> have different base domains" );
fi;
# This eventually should go into the kernel without creating
# a intermediate objects:
for i in [1..Length(srcrows)] do
Expand Down Expand Up @@ -1006,6 +1037,10 @@ InstallMethod( MatElm, "for a zmodnz matrix and two positions",
InstallMethod( SetMatElm, "for a zmodnz matrix, two positions, and an object",
[ IsZmodnZMatrixRep and IsMutable, IsPosInt, IsPosInt, IsObject ],
function( m, row, col, ob )
if ValueOption( "check" ) <> false and
not ( IsInt( ob ) or ob in BaseDomain( m ) ) then
Error( "<ob> must be an integer or in the base domain of <m>" );
fi;
m![ROWSPOS][row]![ELSPOS][col] := Int(ob);
end );

Expand Down
17 changes: 16 additions & 1 deletion lib/matobjplist.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ InstallMethod( NewVector, "for IsPlistVectorRep, a ring, and a list",
[ IsPlistVectorRep, IsRing, IsList ],
function( filter, basedomain, l )
local typ, v;
if ValueOption( "check" ) <> false and not IsSubset( basedomain, l ) then
Error( "the elements in <l> must lie in <basedomain>" );
fi;
typ := MakePlistVectorType(basedomain,IsPlistVectorRep);
v := [basedomain,ShallowCopy(l)];
Objectify(typ,v);
Expand All @@ -63,7 +66,9 @@ InstallMethod( NewMatrix,
"for IsPlistMatrixRep, a ring, an int, and a list",
[ IsPlistMatrixRep, IsRing, IsInt, IsList ],
function( filter, basedomain, rl, l )
local nd, filterVectors, m, e, filter2, i;
local check, nd, filterVectors, m, e, filter2, i;

check:= ValueOption( "check" ) <> false;

# If applicable then replace a flat list 'l' by a nested list
# of lists of length 'rl'.
Expand All @@ -85,6 +90,9 @@ InstallMethod( NewMatrix,
else
m[i] := NewVector( filterVectors, basedomain, l[i] );
fi;
if check and Length( m[i] ) <> rl then
Error( "the rows of <m> must have length <rl>" );
fi;
od;
e := NewVector(filterVectors, basedomain, []);
m := [basedomain,e,rl,m];
Expand Down Expand Up @@ -827,6 +835,13 @@ InstallMethod( MatElm, "for a plist matrix and two positions",
InstallMethod( SetMatElm, "for a plist matrix, two positions, and an object",
[ IsPlistMatrixRep and IsMutable, IsPosInt, IsPosInt, IsObject ],
function( m, row, col, ob )
if ValueOption( "check" ) <> false then
if not ob in BaseDomain( m ) then
Error( "<ob> must lie in the base domain of <m>" );
elif col > m![RLPOS] then
Error( "<col> must be at most <m>![RLPOS]" );
fi;
fi;
m![ROWSPOS][row]![ELSPOS][col] := ob;
end );

Expand Down
4 changes: 2 additions & 2 deletions lib/vec8bit.gi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1094,7 +1094,7 @@ InstallMethod( BaseField, "for a compressed 8bit vector",
InstallMethod( NewVector, "for Is8BitVectorRep, GF(q), and a list",
[ Is8BitVectorRep, IsField and IsFinite, IsList ],
function( filter, f, l )
if not Size(f) in [3..256] then
if ValueOption( "check" ) <> false and not Size(f) in [3..256] then
Error("Is8BitVectorRep only supports base fields with 3 to 256 elements");
fi;
return CopyToVectorRep(l,Size(f));
Expand All @@ -1116,7 +1116,7 @@ InstallMethod( NewMatrix, "for Is8BitMatrixRep, GF(q), an int, and a list",
[ Is8BitMatrixRep, IsField and IsFinite, IsInt, IsList ],
function( filter, f, rl, l )
local m;
if not Size(f) in [3..256] then
if ValueOption( "check" ) <> false and not Size(f) in [3..256] then
Error("Is8BitMatrixRep only supports base fields with 3 to 256 elements");
fi;
m := List(l,ShallowCopy);
Expand Down
2 changes: 1 addition & 1 deletion tst/testinstall/MatrixObj/Eigenvalues.tst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ gap> mat := [[1,2,1],[1,0,1],[0,0,1]];
[ [ 1, 2, 1 ], [ 1, 0, 1 ], [ 0, 0, 1 ] ]
gap> Eigenvalues(Rationals,mat);
[ 2, 1, -1 ]
gap> matObj1 := NewMatrix(IsPlistMatrixRep,GF(5),3,mat);
gap> matObj1 := NewMatrix(IsPlistMatrixRep,GF(5),3,mat*Z(5)^0);
<3x3-matrix over GF(5)>
gap> Eigenvalues(GF(5),matObj1);
[ Z(5)^2, Z(5)^0, Z(5) ]