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

Improve (New)ZeroMatrix, (New)IdentityMatrix, (New)ZeroVector to better reject invalid input #4366

Merged
merged 9 commits into from
Apr 19, 2021

Conversation

fingolfin
Copy link
Member

This is work in progress, but I wanted to have it already visible, so that e.g. @ThomasBreuer has a chance to comment if he wants to. Also, so that I can link to this PR from issues :-).

It should actually be mostly ready, but I need to go over everything once, and there are a few things that should be cleaned up, perhaps a few more tests can be added... We'll see.

@fingolfin fingolfin added kind: bug Issues describing general bugs, and PRs fixing them kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) labels Apr 6, 2021
@ThomasBreuer
Copy link
Contributor

One aim seems to be to get rid of ConvertToVectorRep calls,
to call the responsible low level functions instead where possible,
and to call CopyToVectorRep (which was available only in HPCGAP up to now) otherwise.

(The same would make sense for ConvertToMatrixRep.)

Concerning the extended argument checks and error messages,
wouldn't it be easier to leave them to CONV_VEC8BIT?

@fingolfin fingolfin removed the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Apr 15, 2021
@fingolfin fingolfin changed the title MatrixObj: improve (New)ZeroMatrix, (New)IdentityMatrix (WIP) MatrixObj: improve (New)ZeroMatrix, (New)IdentityMatrix, (New)ZeroVector Apr 15, 2021
@fingolfin
Copy link
Member Author

This is now ready for review: I've added more tests, dropped some stuff, added missing variants of ZeroVector, and grouped everything into hopefully logical commits which should make the motivation for each change clearer. To further aid with that, I add the tests in an early commit, and then for later commits containing fixes, the effect of the fix is visible in how it changes the test file.

@fingolfin fingolfin marked this pull request as ready for review April 15, 2021 00:20
These could become part of a future "conformance test suite" for MatrixObj
implementations.
... using NewZeroMatrix & NewIdentityMatrix.

One could now turn ZeroMatrix & IdentityMatrix into global functions, except
that cvec installs methods for them, for an optimization: There, each
vector/matrix over a certain field and row length stores an immutable
"cvecclass" object; instead of reconstructing it each time, they copy it from
the vector/matrix given as "template" to those variants of ZeroMatrix &
IdentityMatrix which take an "example" object as input.
Even nicer would be to replace them by a single "default" version
of NewIdentityMatrix. Unfortunately, this does not seem possible
for constructors in GAP.
@fingolfin fingolfin force-pushed the mh/ZeroMatrix branch 2 times, most recently from a0d16f0 to 1a17ffc Compare April 15, 2021 00:45
... for IsGF2MatrixRep and Is8Bit2MatrixRep. As a nice bonus, the
changes also are tiny optimizations.
Since IsMatrix has a high rank and IsGF2VectorRep / Is8BitVectorRep objects
are in the filter IsMatrix, the wrong BaseDomain method was selected for such
objects. This reduces performance, and also caused an error with vectors of
length 0.
Now that we have CopyToVectorRep in regular GAP, use it
Copy link
Contributor

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

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

I do not want to request changes (thus we can merge this pull request),
but the following thoughts about details should be discussed further.

  • Explicitly downranking the BaseDomain method for IsRowVector is motivated by the fact that otherwise the (better) method for IsGF2VectorRep has a lower rank.
    I would argue that the methods for IsGF2VectorRep should better be installed for IsRowVector and IsGF2VectorRep since any object in IsGF2VectorRep will be also in IsRowVector.
    One could define IsGF2Vector:= IsRowVector and IsGF2VectorRep, and use this in the method installations.
    This "concurrence situation" will not occur for other VectorObj implementations, where the defining filter really determines the method that will be used.

  • The removal of the "default method" for NewZeroVector is motivated by the fact that constructors cannot have default methods, see the discussion of issue MatrixObj: concerns regarding constructors like NewZeroMatrix, NewVector, etc.  #4398. The documentation which methods are mandatory for a new VectorObj/MatrixObj implementation will depend on the result of this discussion.

@fingolfin
Copy link
Member Author

The main reason I dealt with BaseDomain using -SUM_FLAGS was that the following methods all did the same. But I am happy to have it undone and instead tweak the filters for the BaseDomain methods for Is8BitVectorRep and IsGF2VectorRep as suggested.

However, I wonder why IsRowVector is not automatically implied by IsGF2VectorRep... So I had a look at how we define it:

DeclareRepresentation(
    "IsGF2VectorRep",
    IsDataObjectRep and IsVectorObj, [],
    IsRowVector );

This then lead me to submit issues #4408 and #4409.

@fingolfin fingolfin merged commit 4a96db5 into gap-system:master Apr 19, 2021
@fingolfin fingolfin deleted the mh/ZeroMatrix branch April 19, 2021 14:34
@fingolfin fingolfin added the release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes label Aug 17, 2022
@fingolfin fingolfin changed the title MatrixObj: improve (New)ZeroMatrix, (New)IdentityMatrix, (New)ZeroVector Improve (New)ZeroMatrix, (New)IdentityMatrix, (New)ZeroVector to validate their arguments Aug 17, 2022
@fingolfin fingolfin changed the title Improve (New)ZeroMatrix, (New)IdentityMatrix, (New)ZeroVector to validate their arguments Improve (New)ZeroMatrix, (New)IdentityMatrix, (New)ZeroVector to better reject invalid input Aug 17, 2022
@fingolfin fingolfin added release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes and removed kind: bug Issues describing general bugs, and PRs fixing them release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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