-
Notifications
You must be signed in to change notification settings - Fork 160
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
Support consistency checks for Vector
, Matrix
#5153
Conversation
lib/matobj2.gd
Outdated
@@ -1222,6 +1231,9 @@ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I note that the majority of our existing global options are all-lowercase. I wonder if we have established any rules on this anywhere?
@@ -277,6 +288,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 ) ); |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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?
@ThomasBreuer so should I merge this, or do you plan to make changes based on our discussion from last week? (I am not sure if / which changes are needed right now, if any; but e.g. perhaps |
Yes, I am going to push an update. |
a194209
to
031cdfb
Compare
Vector
, Matrix
Vector
, Matrix
The idea is to admit/force consistency checks in the constructions of vector and matrix objects, via
Vector
,NewVector
,Matrix
,NewMatrix
:One either may want to make sure that the given arguments are consistent (e.g., the entries are really in the given ring, the intended shape of a matrix fits to the given number of columns, and the entries also fit to the intended representation) or one wants to omit such checks if possible.
Some problems have been described in #4884, #5133, #5135.
(Consistency checks will also be needed in operations that change existing vector and matrix objects, such as assignments via
:=
.)I propose to use a global option
Check
that is supported byVector
,NewVector
,Matrix
, andNewMatrix
, as follows:If
Check
isfalse
then the code for methods of these operations is allowed to omit consistency checks, otherwise consistency checks are recommended for methods ofNewVector
andNewMatrix
.(Before that, I had tried the idea to introduce new operations
VectorNC
andNewVectorNC
(likewise for matrices) such thatVector
computes the necessary data for callingNewVector
,VectorNC
computes the necessary data for callingNewVectorNC
,NewVector
first checks for consistency and then callsNewVectorNC
.Up to now, each implementation of a new type of vector objects must provide a
NewVector
method. After the change, aNewVectorNC
method would be needed for each type of vector objects.Due to the fact that
NewVector
andNewVectorNC
must be constructors (in the sense of GAP, i. e., they get created withDeclareConstructor
), we cannot have one default method forNewVector
with the above behaviour.Thus each implementation of a new type of vector objects would have to provide also a
NewVector
method.This idea does not look sensible.)