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

MatrixObj: add a common super representation shared by IsPlistMatrixRep #5143

Open
fingolfin opened this issue Oct 20, 2022 · 2 comments
Open
Labels
gapdays2022-summer Issues and PRs that arose at https://www.gapdays.de/gapdays2022-summer

Comments

@fingolfin
Copy link
Member

Several MatrixObj implementations now share a similar structure: they are positional objects which store a basedomain, number of rows and columns, and a pointer to the elements, in whatever format. (The implementation of upper triangular matrices in PR #5130 optimizes this by not storing the columns as those matrices are always square, but this is a minor optimization for which I am not sure it is worth keeping it -- if one is truly concerned about this, I think then one should change far more about the inner representation.)

There is quite some code duplication because of this: methods for BaseDomain, NrRows, NrCols, ShallowCopy, PostMakeImmutable, MutableCopyMat are basically identical. They also all define a few constants to store the positions of the basedomain/rows/cols/elements.

So perhaps we should define all of these only once and have a kind of super representation IsPositionalMatrixObjRep and then have IsPlistMatrixRep etc. be extensions of this?

Then this:

DeclareRepresentation( "IsPlistMatrixRep",
        IsRowListMatrix and IsPositionalObjectRep
    and IsNoImmediateMethodsObject
    and HasNumberRows and HasNumberColumns
    and HasBaseDomain and HasOneOfBaseDomain and HasZeroOfBaseDomain,
    [] );

might become this:

DeclareRepresentation( "IsPositionalMatrixObjRep",
        IsPositionalObjectRep and IsNoImmediateMethodsObject
    and HasNumberRows and HasNumberColumns
    and HasBaseDomain and HasOneOfBaseDomain and HasZeroOfBaseDomain,
    [] );

DeclareRepresentation( "IsPlistMatrixRep",
        IsRowListMatrix and IsPositionalMatrixObjRep,
    [] );

In a quick test that seems to work.

Does it sound reasonable, @ThomasBreuer @wucas @AnnaKDS @danielrademacher ?

Then we could add this into a file, say matobj_positional.gd together the definitions of constants like BDPOS etc. (renamed suitably).

If this seems reasonable I could give it a go this afternoon.

@fingolfin fingolfin added the gapdays2022-summer Issues and PRs that arose at https://www.gapdays.de/gapdays2022-summer label Oct 20, 2022
@danielrademacher
Copy link
Contributor

I would say that this is a good idea. This probably provides less similar code because one can define more optimised generic functions.

We should then also mention that in the documentation such that the implementation of new types can be orientated according to this structure.

@wucas
Copy link
Contributor

wucas commented Oct 20, 2022

I think that idea is great. Reducing code duplication is always nice of course but it makes it easier to implement new types as well.

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
Projects
None yet
Development

No branches or pull requests

3 participants