-
Notifications
You must be signed in to change notification settings - Fork 161
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
Overhaul IsPlistMatrixRep and IsPlistVectorRep #2973
base: master
Are you sure you want to change the base?
Conversation
lib/matobjplist.gi
Outdated
# [ IsPlistMatrixRep, IsFunction ], | ||
# function( m, f ) | ||
# return List(m![ROWSPOS],f); | ||
# end ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MatrixObj are not lists, so remove these. If one wants to extract the rows or columns as VectorObj, then we should add GetColumnVectors
/ GetRowVectors
operations (better names pending...)
lib/matobjplist.gi
Outdated
fi; | ||
od; | ||
l := a![ROWSPOS]*b![ROWSPOS]; | ||
#l := PROD_LIST_SCL_DEFAULT(a![ROWSPOS],b![ROWSPOS]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we use the optimized matrix multiplication functions from the kernel, which is part of the reason for the performance improvements.
68c1172
to
e1a1fb6
Compare
Codecov Report
@@ Coverage Diff @@
## master #2973 +/- ##
===========================================
- Coverage 84.65% 72.29% -12.36%
===========================================
Files 698 635 -63
Lines 345626 308665 -36961
===========================================
- Hits 292582 223150 -69432
- Misses 53044 85515 +32471
|
e1a1fb6
to
b04f4b6
Compare
dcf9ac2
to
cb79cdf
Compare
a323d50
to
05693a1
Compare
05693a1
to
3772086
Compare
3772086
to
5141cfa
Compare
5141cfa
to
0e7599b
Compare
0e7599b
to
f8f3ea1
Compare
f8f3ea1
to
f34dc7b
Compare
f34dc7b
to
b52529a
Compare
738c096
to
8a085dd
Compare
7fa02fd
to
fe334ea
Compare
I think this is getting close to being mergeable. I am sure there are bugs in my changes, but to flush those out, we should write more and better systematic tests for the interfaces. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had a look up until line 711 and left a few comments.
m := []; | ||
elif IsVectorObj(l[1]) then | ||
# list of vectors | ||
# TODO: convert each IsVectorObj to a plist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds to me like you want to turn each entry of l
into IsPlistRep
. That can't be right, or am I misunderstanding something? The documentation states, that a plist of IsPlistVectorRep
objects should be stored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A primary point of this PR is to change that, i.e.: do not store a list of vector objects, but rather store an old style matrix. This avoids a ton of overhead in both memory usage and performance. Before this PR, IsPlistMatrixRep
was hopelessly slow compared to old-style matrices.
# FIXME/TODO: should the following test be always performed | ||
# or only at a higher assertion level? | ||
Assert(0, ForAll(m, row -> Length(row) = ncols)); | ||
Assert(0, ForAll(m, row -> ForAll(row, x -> x in basedomain))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentations of IsPlist*Rep
do not state that the entries must be in basedomain
. So IMO either we remove this Assert
or add corresponding statements into the docs.
elif Is8BitVectorRep(l) then | ||
PLAIN_VEC8BIT(v); | ||
fi; | ||
v := PlainListCopy(l); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I take it PlainListCopy
respects the GF2 and 8Bit vector reps? (This isn't tested accordingn to codecov)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean with "respects" them? It returns a new plain list with the content of the given list, and it works for arbitrary lists, including GF2 and 8bit vector reps.
l := List([1..rows],i->ZeroVector(rows,t)); | ||
function( rows, m ) | ||
local i,l,o,res; | ||
l := List([1..rows],i->ListWithIdenticalEntries(rows, Zero(m![BDPOS]))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then the entries of l
would be plain lists although I thought they should be in IsPlistVectorRep
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added some specific comments in the code.
Two more general remarks:
Several v![BDPOS]
have been changed to BaseDomain(v)
. If this is a general suggestion then should it be applied everywhere?
The documentation of IsPlistMatrixRep
states that a plain list of IsPlistVectorRep
objects is stored, should this be changed?
lib/matobjplist.gd
Outdated
@@ -45,7 +45,7 @@ | |||
## <#/GAPDoc> | |||
## | |||
DeclareRepresentation( "IsPlistVectorRep", | |||
IsVectorObj and IsPositionalObjectRep, [] ); | |||
IsVectorObj and IsPositionalObjectRep and IsNoImmediateMethodsObject, [] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IsNoImmediateMethodsObject
is currently undocumented. I expect that it will be useful also for other new kinds of vector and matrix objects. Thus it should become documented, and the documentation of vector and matrix objects should mention it, in a section on "performance hints".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, why not. Open an issue for that idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #4529.
gap> NewMatrix(IsPlistMatrixRep, Integers, 3, [ ] ); Display(last); | ||
<0x3-matrix over Integers> | ||
<0x3-matrix over Integers: | ||
]> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The opening square bracket seems to be missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, fixed now by revised printing.
# FIXME/TODO: should the following test be always performed | ||
# or only at a higher assertion level? | ||
Assert(0, ForAll(m, row -> Length(row) = ncols)); | ||
Assert(0, ForAll(m, row -> ForAll(row, x -> x in basedomain))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this a more general question about the operation? Do we perhaps need two operations, a NC
variant and one that performs the checks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is ultimately a general question. As are many of the other TODOs. Note that similar questions arise e.g. for \+
for matrices (should we check and enforce that the dimensions agree? or do we want to allow one matrix to be "smaller" than the other? Or do we not want to check at all and rely on the user?
For other binary operations, should we check that the base domains are identical? Or possibly equal (slower, but more permissive)? Or perhaps we should allow adding matrices with different base domains as long as there is a common parent of the base domains (so adding a GF(4) and a GF(8) matrix might produce a GF(64) matrix). If so, what mechanism should be used to determine that "parent" domain?
And if we do perform any such checks, do we need an NC version of "+"? And how would one name that?
There's more. I am also pretty sure we already listed some of these somewhere.
od; | ||
# TODO: should we verify that Length(v) = NrRows(m)? | ||
res := ZeroVector(NrCols(m),v); | ||
res![ELSPOS] := v![ELSPOS] * m![ROWSPOS]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say it depends on the definition of arithmetic operations for vector and matrix objects:
If we say that the result is undefined if the dimensions do not match then no check is necessary.
If we promise an error message if the dimensions do not match then we have to check.
(Without check, the result object is at least "internally consistent", that is, the claimed number of columns equals the length of the stored list.)
lib/matobjplist.gi
Outdated
Append(m![ROWSPOS],n![ROWSPOS]); | ||
end ); | ||
|
||
# FIXME: does ShallowCopy even make sense for a matrixobj??? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In matobj2.gd
, we define ShallowCopy
for objects in IsRowListMatrix
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spot on. Thing is, in my mind, IsPlistMatrixRep
should not have been in IsRowListMatrix
, but of course it still was when you wrote that review remark. I simply forgot to make that change... now done, and IsRowPlistMatrixRep
added. And then I changed this ShallowCopy
method to require IsRowPlistMatrixRep
.
lib/matobjplist.gi
Outdated
trans := TransposedMatMutable(m![ROWSPOS]); | ||
|
||
|
||
# FIXME: using TypeObj(m) below is probably wrong, as it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are many places in this file where a TypeObj
value of the argument is used in Objectify
, and where this type may be wrong.
# FIXME: if the base domain is not a field, e.g. Integers, | ||
# then the inverse we just computed may not be defined over | ||
# that base domain. Should we detect this here? Or how else | ||
# will we deal with this in general? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is first of all a question about what the operation NewMatrix
promises. Perhaps we need NewMatrix
and NewMatrixNC
? (And then the question is whether we call NewMatrix
or NewMatrixNC
here.)
@ssiccha @ThomasBreuer thank you for your feedback @ThomasBreuer you wrote:
Yes, thank you. In fact, there is more in it that needs to be changed: it implies that HOWEVER another useful application for So here we have a clash of goal. So my thought here now is to introduce |
Err, I've started to add |
fe334ea
to
c2ff178
Compare
c2ff178
to
fefb213
Compare
fefb213
to
343685a
Compare
- change `IsPlistMatrixRep` to *not* be a row list type; so instead of storing a list of `IsPlistVectorRep` instances, they now store an old-style matrix in `IsMatrix and IsPlistRep`. This in turn allows to simplify and optimize a lot of code, e.g. matrix arithmetic can now just dispatch to that for plain lists. - use `BindConstant` on `BDPOS` etc. for best performance - set `IsNoImmediateMethodsObject` filter for both for better performance - rename `EMPOS` and `RLPOS` to `NUM_ROWS_POS` and `NUM_COLS_POS` - allow semirings as basedomain - fix various vector/matrix constructors to copy their inputs lists instead of reusing them inside (that can lead to bad bugs) - removed a bunch of (now) redundant methods that didn't seem to provide any benefits (such as improved performance) - ...
This fixes printing of 0 x n matrices
343685a
to
be1261d
Compare
Progress towards issue #2148 : This PR is an incomplete proof of concept (see #2776 for its genesis), I am putting it out here to make sure people are aware of this possible direction, and in the vague hope that somebody else might help out with it.
The idea is to tune
IsPlistMatrixRep
as much as possible so that it becomes a viable replacement for lists-of-lists (= lol) "matrices". Indeed, with this hackish POC, I already benchmarked some cases where theIsPlistMatrixRep
code is faster than lists-of-lists, probably in parts because of avoided method selection overhead; another part is that the optimized code benefits from kernel optimizations for multiplication of lol matricesTo give an idea about the performance of
IsPlistMatrixRep
before and after this PR, I run some mini benchmarks, using the benchmarking code in this gist, plus the following code:The results of this are as follows with
master
(using commit 5b7263c from 2019-07-11), on a Ubuntu server in Gießen:With this PR (commit cb79cdf), the numbers for
Is8BitMatrixRep
andIsPlistRep
stay essentially the same, but forIsPlistMatrixRep
improve to more or less match those forIsPlistRep
(they are a bit slower, but not that much):Of course proper benchmarks would include a range of matrix sizes and ground fields; more interesting / realistic operations; and graph all of that nicely...