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

Rank down MatrixObj emulation methods #5503

Merged
merged 1 commit into from
Sep 18, 2023

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Sep 4, 2023

Reduce the rank of NumberRows and NumberColumns methods that are meant to
emulate the MatrixObj interface for classic plist-of-list matrices.

These methods were in some cases ranked higher than their counterparts for compressed matrices, leading to unnecessary overhead for e.g. NrRows when invoked on a compressed matrix.

@fingolfin fingolfin added topic: library release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Sep 4, 2023
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.

For NumberRows and NumberColumns, I agree that these generic methods had a too high rank, and ranking them down is an improvement.
For the other method installations in question, IsPlistRep is explicitly required, thus the methods are not applicable to compressed matrices. What do I misunderstand?

@ThomasBreuer
Copy link
Contributor

The test failures are interesting.

NewVector( IsPlistRep, GF(3), [ Z(3) ] ) is not supported.
With the current pull request, this causes an error when one calls RowsOfMatrix( [ [ Z(3) ] ] ), because the default method for IsMatrixOrMatrixObj calls NewVector in the above form.
Without the current pull request, the RowsOfMatrix method for IsPlistRep matrices is called instead.

One could argue that a NewVector method for the abovementioned situation should be provided such that we can use the IsMatrixOrMatrixObj default for RowsOfMatrix.
On the other hand, NewVector is formally defined only for situations where a vector object is returned, and IsPlistRep objects are not in IsVectorObj.

Thus either the IsPlistRep methods for "higher" operations such as RowsOfMatrix must be ranked above the IsMatrixOrMatrixObj methods, or we need a NewVector method for IsPlistRep, and NewVector must be defined for the more general case that an object either in IsVectorObj or in IsPlistRep is returned.

If we decide that we take the latter point of view then I can create a pull request that provides the missing methods.

Reduce the rank of `NumberRows` and `NumberColumns` methods that are meant to
emulate the MatrixObj interface for classic plist-of-list matrices.

These methods were in some cases ranked higher than their counterparts
for compressed matrices, leading to unnecessary overhead for e.g.
`NumberRows` when invoked on a compressed matrix.
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.

Fine.
As we had discussed today, we do not require NewVector etc. methods for IsPlistRep matrices.

@ThomasBreuer ThomasBreuer merged commit 7ee7ce9 into gap-system:master Sep 18, 2023
22 checks passed
@fingolfin fingolfin deleted the mh/mat-method-rank branch September 18, 2023 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants