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

Support consistency checks for Vector, Matrix #5153

Merged
merged 3 commits into from
Nov 6, 2022

Conversation

ThomasBreuer
Copy link
Contributor

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 by Vector, NewVector, Matrix, and NewMatrix, as follows:
If Check is false then the code for methods of these operations is allowed to omit consistency checks, otherwise consistency checks are recommended for methods of NewVector and NewMatrix.

(Before that, I had tried the idea to introduce new operations VectorNC and NewVectorNC (likewise for matrices) such that

  • Vector computes the necessary data for calling NewVector,
  • VectorNC computes the necessary data for calling NewVectorNC,
  • NewVector first checks for consistency and then calls NewVectorNC.
    Up to now, each implementation of a new type of vector objects must provide a NewVector method. After the change, a NewVectorNC method would be needed for each type of vector objects.
    Due to the fact that NewVector and NewVectorNC must be constructors (in the sense of GAP, i. e., they get created with DeclareConstructor), we cannot have one default method for NewVector 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.)

@ThomasBreuer ThomasBreuer added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: library release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes gapdays2022-summer Issues and PRs that arose at https://www.gapdays.de/gapdays2022-summer labels Oct 20, 2022
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
Copy link
Member

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 ) );
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?

@fingolfin
Copy link
Member

@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 Check renamed to check?)

@ThomasBreuer
Copy link
Contributor Author

Yes, I am going to push an update.

@fingolfin fingolfin merged commit 7f08ed2 into gap-system:master Nov 6, 2022
@ThomasBreuer ThomasBreuer deleted the TB_Vector_Check branch November 7, 2022 08:31
@fingolfin fingolfin changed the title support consistency checks for Vector, Matrix Support consistency checks for Vector, Matrix Jan 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gapdays2022-summer Issues and PRs that arose at https://www.gapdays.de/gapdays2022-summer kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants