WIP: positional matrix obj rep #5217
Conversation
fingolfin
force-pushed
the
mh/IsPositionalMatrixObjRep
branch
from
08d107b
to
e1c3728
Compare
|
I think I will rename Hence the idea to rename it to Thoughts on this, anyone? @wucas @ThomasBreuer @ChrisJefferson (Note: this won't be in this PR, I need to get that finished first; but the above notion came up precisely while digging through the code and trying to figure out how to adapt it to use |
fingolfin
force-pushed
the
mh/IsPositionalMatrixObjRep
branch
2 times, most recently
from
309f60e
to
dfdeb63
Compare
There was a problem hiding this comment.
The idea to provide a generic implementation of vector and matrix objects via positional objects is nice.
I have inserted a few comments.
Eventually the new representations should become documented, and then the files lib/matobjnz.g* can be changed in the same way as lib/matobjplist.g*.
(Doing this now would create conflicts with pull request #5254.)
| res![VEC_DATA_POS] := ShallowCopy(v![VEC_DATA_POS]); | ||
| res := Objectify(TypeObj(v), res); | ||
|
|
||
| # 'ShallowCopy' MUST return a mutable object if such an object exists at all! |
There was a problem hiding this comment.
My interpretation of ShallowCopy is that it returns its argument if this is not copyable.
We could restrict the method installation to IsPositionalVectorRep and IsCopyable.
And perhaps we should add an explicit reference to IsCopyable to the documentation of ShallowCopy.
| return res; | ||
| end ); | ||
|
|
||
| # StructuralCopy works automatically |
There was a problem hiding this comment.
StructuralCopy is not an operation, so we cannot do anything for it here.
| res![MAT_DATA_POS] := ShallowCopy(m![MAT_DATA_POS]); | ||
| res := Objectify(TypeObj(m), res); | ||
|
|
||
| # 'ShallowCopy' MUST return a mutable object if such an object exists at all! |
There was a problem hiding this comment.
see the comment about ShallowCopy for IsPositionalVectorRep
|
|
||
| # TODO: document this | ||
| DeclareRepresentation( "IsPositionalVectorRep", | ||
| IsVectorObj and IsPositionalObjectRep |
There was a problem hiding this comment.
and IsCopyable if we adopt the idea to make this explicit
|
|
||
| # TODO: document this | ||
| DeclareRepresentation( "IsPositionalMatrixRep", | ||
| IsMatrixObj and IsPositionalObjectRep |
There was a problem hiding this comment.
and IsCopyable if we adopt the idea to make this explicit
fingolfin
force-pushed
the
mh/IsPositionalMatrixObjRep
branch
from
dfdeb63
to
0b590c3
Compare
fingolfin
force-pushed
the
mh/IsPositionalMatrixObjRep
branch
from
0b590c3
to
20dd974
Compare
FIXME: actually the "generic" ShallowCopy method is wrong, as it e.g. doesn't reset IsZero etc -- if we want to keep this, we need something like a helper to produce a "basic" typeobj for the given basedomain.
fingolfin
force-pushed
the
mh/IsPositionalMatrixObjRep
branch
from
20dd974
to
32f9269
Compare
The plan for this PR is to provide two incomplete representations for vector obj and matrix obj, on which many other implementations (such as the existing plist matrix obj) can be based; this is meant to reduce work for new implementation, reduce code duplication, and provide some uniformity.
Part of the work is to take constants like
BDPOSand give them a better name. While doing this, I tried to reorder them, but discovered this isn't easy because their values are implicitly used in aObjectifycalls (at least 70 are relevant for this). Since they also represent further code duplication, I plan to put them into a few helper functions, to reduce the number to 4 or so (one each for matrix vs. vector, and for plist rep vs. zmodnz rep). But then @ThomasBreuer pointed out the extra overhead this indirection causes... While I am not overly worried about this (in the worst case I can rewrite these as kernel functions), that made us think about the general impact of objection creation overhead for matrix obj vs. old-style matrices... So my work plan for this PR right now is this:benchmark matrix and vector creation times; add the code for this into the repo somewhere; then act on the result. The idea is to compare at least these:
factor out
Objectifycallsreally switch to the new constants (possibly not reordered for now)