Merge some MatrixObj work into master #2640

fingolfin · 2018-07-12T14:12:29Z

Last year, we made some progress on MatrixObj. Sadly, we got stalled again. But I think even the partial work we did would already be useful.

I took the MatrixObj2 branch, and looked through it, and cleaned it up a lot. I also dropped (for now, this is only temporary) a few changes that I think are potentially more controversial (such as @ssiccha MultRowVector to MultVector change). Assuming this PR gets merged, I would then prepare a new branch and PR (or possibly multiple) with all the remaining work, which then can either merge, too, or continue to work on.

Some TODOs (?) before merge:

the tests in tst/test-matobj can be run via tst/test-matobj.g, but this is not currently hooked up to Travis. Is there any reason why we don't just move these tests into testinstall and/or (for very slow parts) into teststandard?
writing a summary of the changes in here for the GAP 4.10 release notes
running package tests against this PR, to see if it breaks any of them (maybe @alex-konovalov can help with that?)

codecov · 2018-07-13T10:43:56Z

Codecov Report

Merging #2640 into master will decrease coverage by 0.01%.
The diff coverage is 54.23%.

@@            Coverage Diff             @@
##           master    #2640      +/-   ##
==========================================
- Coverage   75.48%   75.47%   -0.02%     
==========================================
  Files         478      478              
  Lines      241601   243156    +1555     
==========================================
+ Hits       182382   183524    +1142     
- Misses      59219    59632     +413

Impacted Files	Coverage Δ
lib/matblock.gi	`88.19% <ø> (ø)`	⬆️
hpcgap/lib/vecmat.gi	`64.52% <ø> (+0.91%)`	⬆️
lib/memory.gi	`2.59% <ø> (ø)`	⬆️
lib/vecmat.gi	`68.55% <ø> (+0.52%)`	⬆️
lib/mat8bit.gi	`68.31% <ø> (+1.23%)`	⬆️
lib/vspcrow.gi	`78.82% <100%> (ø)`	⬆️
lib/vspcmat.gi	`92.47% <100%> (ø)`	⬆️
lib/ctblpope.gi	`73.85% <100%> (ø)`	⬆️
lib/ringsc.gi	`71.99% <100%> (ø)`	⬆️
lib/zlattice.gi	`77.27% <100%> (ø)`	⬆️
... and 29 more

frankluebeck · 2018-07-13T12:40:27Z

I support to proceed as suggested here. Most changes will not influence existing GAP code and it would be good to start over from a master branch that contains most of the changes from the MatrixObj branch.

Looking through the diffs I did not notice anything problematic. Moving the files from tst/test-matobj to
tst/testinstall seems ok.

Currently, some tests of TraceMat fail. This could be fixed by a method in matobjplist.gi:

InstallMethod( \[\], "for a plist matrix and two positive integers",
  [ IsPlistMatrixRep, IsPosInt, IsPosInt ],
  function( m, r, c )
    return m![ROWSPOS][r][c];
  end );

Or should we also merge the more efficient kernel code that provides this functionality?

fingolfin · 2018-07-13T12:57:42Z

The failing tests are a very recent regression (the previous build was fine), and I'll fix it. I also already moved the tests into testinstall, and added a few more.

fingolfin · 2018-07-13T13:45:38Z

Actually, the TraceMat one is already fixed by adding methods for [i,j] resp. [i,j]:=; both for IsPlistMatrixRep, and also generic ones for IsMatrixObj which delegate to MatElm and SetMatElm.

The current test failure is something new, I'll investigate

ThomasBreuer

The changes are fine.

There are just two things which I do not understand:
The file hpcgap/lib/vec8bit.gi gets changed
but there are no changes for lib/vec8bit.gi;
as far as I see, the current code in lib/vec8bit.gi still contains ConvertToVectorRepNC calls
which we want to get rid of.
And what is the reason to replace, in hpcgap/lib/vec8bit.gi,
the one-line statements involving CopyToVectorRep and an arithmetic operation by two statements?

fingolfin · 2018-07-13T21:51:40Z

There were changes in hpcgap/lib/vec8bit.gi to align it closer to the code in lib/vec8bit.gi, making the actual differences (ConvertToRep vs. CopyRep) stand out more. Anyway, while it is on the old MatrixObj2 branch, it doesn't really fit into this PR, so I dropped it.

markuspf · 2018-07-13T22:16:08Z

I enthusiastically support merging this; it's useful code that shouldn't lie around somewhere bitrotting.

I haven't really worked on this code, so I don't have in-depth knowledge of what is added/changed.

I remember that MatrixObjs have been part of GAP for a long time already (and are used in packages), but I see that now they are documented and tested, so we should make extra-sure that (if we put this into 4.10), we're ready to continue supporting this code.

fingolfin · 2018-07-14T14:24:58Z

This passes all tests now. But it would be good to run the package tests against this, too, as I wrote above, before merging; let's wait to hear from @alex-konovalov on this.

BTW, this PR add methods for m[i,j] and m[i,j]:=x which delegate to the old SetMatElm and MatElm. This is meant to allow people to convert code which uses MatElm and SetMatElm to instead use the new [i,j] syntax instead. But on the long run, I think we should drop MatElm and SetMatElm completely, or at least reverse the "fallback": I.e. in the future, we would have a MatElm default method which falls back to m[i,j]. But to get there, all packages implementing MatrixObj need to provide methods for the new access operations directly. I just added a patch for cvec that will be in the next cvec release. That leaves homalg, which uses MatElm all over the place. Perhaps @mohamed-barakat can start a push to convert it to use [i,j] instead everywhere -- with a regex search and replace, that should be doable with half an hour of work, and only needs GAP 4.9 to work, I think, nothing from this PR (as long as both MatElm/SetMatElm methods and methods for \[\] and \[\]\:\= with two indices are provided).

olexandr-konovalov · 2018-07-18T11:52:36Z

@fingolfin hear this - doing tests now.

fingolfin · 2018-08-09T20:28:44Z

@alex-konovalov any news on this?

olexandr-konovalov · 2018-08-14T10:50:58Z

I have checked test logs from July 18th and I don't see diffs in package tests which are different from diffs that I've seen in their tests run with the GAP master branch. Since that was almost a month ago, may be worth to rebase this PR and run another round of tests to be sure.

These delegate to MatElm resp. SetMatElm, so that old MatrixObj implementations which only provide these old accessors can be used with the new m[i,j] syntax. Ideally, these will be removed again one day in the future, together with MatElm and SetMatElm.

Note that the cvec package also installs methods for some of these, and should possibly be adjusted to avoid them We should also think about replacements. Perhaps there should be PositionLastNonZeroRow and PositionLastNonZeroColumn methods? Finally, why is PositionLastNonZero for (row) vectors in vecmat.gi, which contains code for GF2 vectors and matrices?

Also add ZeroMatrix and IdentityMatrix variants, and add lots of comments.

This makes `Vector` the main interface function to create vector objects.

- get rid of IsCheckingVector and IsCheckingMatrix - update internal doc: - NewMatrix now additionally accepts a flat list - remove redundant comments

changed most library calls to PositionNot added attributes OneOfBaseDomain, ZeroOfBaseDomain, and generic methods for plist vectors and plist matrices

IsCheckingVector and -Matrix do checks like: - when doing v * s, check whether s in BaseDomain(v) holds. this can potentially be very expensive - check whether accessing or binding to illegal entries - check whether dimensions of matrices and vectors fit when doing AddRowVector etc. \* for IsPlist*Rep already checks whether dimensions match and whether the base domains are identical. Instead some checks could be done (like index out of bounds checks) on the kernel level.

and dealt with some side-effects: - changed calls of `MatElm(m,i,j)` to `m[i,j]` where `MatElm` methods for plain lists of plain lists are missing - changed those method installations from `InstallMethod` to `InstallOtherMethod` that currently cause a warning about matching multiple declarations; in a later step, some of these declarations will get merged - changed calls of `BaseDomain` to calls of `OneOfBaseDomain` or `ZeroOfBaseDomain` where only the one or zero element are needed - added comments to `lib/matobj1.gd`

fingolfin · 2018-08-14T11:40:05Z

Thanks. Rebased now, let's see how the tests fare.

fingolfin · 2018-08-15T10:07:12Z

@alex-konovalov could you please run another round of extended tests, so that we can ideally merge this ASAP? Thanks!

olexandr-konovalov · 2018-08-15T10:43:27Z

@fingolfin I've triggered extended tests yesterday, so I've just checked the outcome. I will be happy to merge this now.

fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements topic: library labels Jul 12, 2018

fingolfin requested review from mohamed-barakat, markuspf, olexandr-konovalov, frankluebeck, ssiccha and ThomasBreuer July 12, 2018 14:12

fingolfin force-pushed the mh/MatrixObj-partial branch from d4b4903 to fb9314d Compare July 13, 2018 10:43

fingolfin force-pushed the mh/MatrixObj-partial branch from fb9314d to be5fda4 Compare July 13, 2018 11:31

fingolfin mentioned this pull request Jul 13, 2018

ProjectiveActionOnFullSpace: use list of vectors as domain #2641

Merged

ThomasBreuer reviewed Jul 13, 2018

View reviewed changes

fingolfin force-pushed the mh/MatrixObj-partial branch from be5fda4 to f2ea1ed Compare July 13, 2018 21:25

markuspf approved these changes Jul 13, 2018

View reviewed changes

fingolfin and others added 6 commits August 14, 2018 13:39

MatrixObj: minor changes in matobj.xml

71f0f2e

MatrixObj: added TODOs & notes from meeting in Aachen 2017

b5aefb0

MatrixObj: replace MakeMatrix by additional Matrix variants

d1f4992

Also add ZeroMatrix and IdentityMatrix variants, and add lots of comments.

MatrixObj: handle flat entry list in NewMatrix

4dcc361

frankluebeck and others added 10 commits August 14, 2018 13:39

MatrixObj: more declarations of Vector and generic methods

3b066d5

This makes `Vector` the main interface function to create vector objects.

MatrixObj: further work on constructors

d07e221

- get rid of IsCheckingVector and IsCheckingMatrix - update internal doc: - NewMatrix now additionally accepts a flat list - remove redundant comments

MatrixObj: add docs, tests and default impls for several methods

bb83528

MatrixObj: install generic DimensionsMat method

eba251d

MatrixObj: if possible use PositionNonZero not PositionNot

cc2dd08

changed most library calls to PositionNot added attributes OneOfBaseDomain, ZeroOfBaseDomain, and generic methods for plist vectors and plist matrices

MatrixObj: Doc BaseDomain, Length

d95df3e

MatrixObj: add IsPlistMatrixRep methods for [,] and [,]:=

7460426

MatrixObj: some more tests

b3cd252

fingolfin force-pushed the mh/MatrixObj-partial branch from f2ea1ed to c193f63 Compare August 14, 2018 11:39

fingolfin added the release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes label Aug 14, 2018

olexandr-konovalov approved these changes Aug 15, 2018

View reviewed changes

fingolfin merged commit 6177a64 into gap-system:master Aug 15, 2018

fingolfin deleted the mh/MatrixObj-partial branch August 15, 2018 19:09

This was referenced Aug 15, 2018

Added file dev/TODO_matobj.txt #2708

Merged

Rename MultRowVector to MultVector #2709

Merged

markuspf mentioned this pull request Aug 23, 2018

Merging #2640 broke Semigroups #2737

Closed

This was referenced Sep 5, 2018

Downrank methods for Matrix and BaseDomain introduced in #2640 to unbreak Semigroups on master #2754

Merged

Rename Matrix to something else semigroups/Semigroups#512

Closed

fingolfin added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Oct 25, 2018

wilfwilson mentioned this pull request Jan 28, 2021

Revert accidentally-reintroduced downranking of a MatrixObj method #4237

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge some MatrixObj work into master #2640

Merge some MatrixObj work into master #2640

fingolfin commented Jul 12, 2018 •

edited by ssiccha

Loading

codecov bot commented Jul 13, 2018 •

edited

Loading

frankluebeck commented Jul 13, 2018

fingolfin commented Jul 13, 2018

fingolfin commented Jul 13, 2018

ThomasBreuer left a comment

fingolfin commented Jul 13, 2018

markuspf commented Jul 13, 2018

fingolfin commented Jul 14, 2018

olexandr-konovalov commented Jul 18, 2018

fingolfin commented Aug 9, 2018

olexandr-konovalov commented Aug 14, 2018

fingolfin commented Aug 14, 2018

fingolfin commented Aug 15, 2018

olexandr-konovalov commented Aug 15, 2018

Merge some MatrixObj work into master #2640

Merge some MatrixObj work into master #2640

Conversation

fingolfin commented Jul 12, 2018 • edited by ssiccha Loading

codecov bot commented Jul 13, 2018 • edited Loading

Codecov Report

frankluebeck commented Jul 13, 2018

fingolfin commented Jul 13, 2018

fingolfin commented Jul 13, 2018

ThomasBreuer left a comment

Choose a reason for hiding this comment

fingolfin commented Jul 13, 2018

markuspf commented Jul 13, 2018

fingolfin commented Jul 14, 2018

olexandr-konovalov commented Jul 18, 2018

fingolfin commented Aug 9, 2018

olexandr-konovalov commented Aug 14, 2018

fingolfin commented Aug 14, 2018

fingolfin commented Aug 15, 2018

olexandr-konovalov commented Aug 15, 2018

fingolfin commented Jul 12, 2018 •

edited by ssiccha

Loading

codecov bot commented Jul 13, 2018 •

edited

Loading