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

Add a minimal matrix object implementation #5152

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wucas
Copy link
Contributor

@wucas wucas commented Oct 20, 2022

This PR adds a matrix object IsMinimalMatrixRep. Internally it represents the matrix as a flat list in row-major form. It only the required methods.

This is intended to be used as a minimal example and in order to test code, in particular the generic methods.

Regarding tests, if nothing obvious is wrong I'd like to merge this asap and then use it with the new test suite. Any errors could be fixed then.

This resolves Issue #5142.

@wucas wucas added 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 gapdays2022-summer Issues and PRs that arose at https://www.gapdays.de/gapdays2022-summer labels Oct 20, 2022
@wucas
Copy link
Contributor Author

wucas commented Oct 20, 2022

Feedback on whether or not to have this in the release notes is welcome.

@ChrisJefferson
Copy link
Contributor

This code seems in a basically good state. Obviously tests for it, and using it, are important but can be part of future work.

Only one thought I have now, before this is merged -- given that most people don't read manuals, I worry that people might see IsMinimalMatrixRep and think it is small, or minimal, and start using it. At the moment, that wouldn't be a completely stupid choice, as it does compress the matrix quite well.

As it might get slower (and less useful) as tests are added to it, I might give it a more explicit name, like IsExampleMinimalMatrixRep (other names OK of course, just something to make it clear people shouldn't be using it).

@wucas
Copy link
Contributor Author

wucas commented Oct 20, 2022

@ChrisJefferson Thanks for the suggestion. I didn't think of that way to understand the name but it is sensible to make it absolutely clear in the name that it is not intended for usage in computations. I chose the name IsMinimalExampleMatrixRep.

@fingolfin
Copy link
Member

Hint: if you replace This takes care of Issue #5142. by This resolves #5142. then it will automatically close that issue when this PR is closed (we can also activate this by manually "linking" the issue and PR, but I think it's good to know about the possibility to activate this with keywords, it's IMHO quite useful.

lib/matobjminimal.gd Outdated Show resolved Hide resolved
lib/matobjminimal.gi Outdated Show resolved Hide resolved
@wucas wucas force-pushed the lw/minimal-matobj branch 2 times, most recently from 9824dbc to 4943105 Compare October 21, 2022 07:43
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants