Compile more operations in ModulePresentationsForCAP #932
Conversation
Codecov ReportBase: 76.88% // Head: 76.84% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #932 +/- ##
==========================================
- Coverage 76.88% 76.84% -0.05%
==========================================
Files 496 494 -2
Lines 61043 60092 -951
==========================================
- Hits 46931 46175 -756
+ Misses 14112 13917 -195
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
Thanks for this start! I guess it's best we discuss this verbally during our next meeting. |
|
Results of our discussion:
This is actually faster than the old version -> use it :-)
Changing
We can precompile the standard version and keep the special cases on top.
This additional check should be removed in CategoryOfRows because any matrix comparison algorithm should do this anyway. Notes for the internal hom:
|
|
Addendum (also already discussed):
|
TKuh
force-pushed
the
module_pres_compiled
branch
from
d665138 to
504bda9
Compare
|
Sorry, that this took such a long time. I'll take care of it now.
Great. :)
I'm having some trouble with this. I remember, that the compiled
I understand this as "remove only the first case in
I searched CategoryOfRows (and FreydCategory) for such a check, but I can't find the place, where this originates. :(
Done.
Here are the runtimes I get with
|
No worries!
I don't think this is true: InternalHomOnObjects/Morphisms is primitively installed. So there is no way around checking that this implementation is compatible with changing the order of the
Yes!
This is unclear to me right now -> keep it in mind for now.
Doesn't look bad, but there seems to be something systematically going on. Can you pinpoint which operations are slower? Either by commenting things in or out, or by using timing statistics? (search for "Timing statistics" in the documentation of CAP) |
There is one problem I can spot. If it does make a difference, then I guess one could either switch the arguments of the calls to
Are these algorithm written down somewhere? I couldn't find it in Sebastians PhD thesis. I would like to understand it mathematically, to see, if my talk is garbage. :) |
Have you checked what the corresponding code in FreydCategory does? If it also differs exactly at this point, you can just port the change over and check that it does not affect the timings.
They should be here: https://arxiv.org/abs/1909.00172. |
I can't prove if the code in There is this comment in Freyd which explains Now, in TensorProductOnMorphisms( CategoryOfRowsMorphism( TransposedMatrix( UnderlyingMatrix( a ) ) ), b ), I purposely leave out some details here in case you dig through the derivations as well. I only want to emphasize the final matrix operations. Now this in turn boils down to: KroneckerMat( TransposedMatrix( UnderlyingMatrix( a ) ), UnderlyingMatrix( b ) )This is probably what happens in the construction of CAP_project/ModulePresentationsForCAP/gap/ModulePresentationObject.gi Lines 224 to 230 in ab7d994 And I guess it corresponds to this line in Freyd: CAP_project/FreydCategoriesForCAP/gap/FreydCategory.gi Lines 356 to 357 in ab7d994 So this part seems fine. On the other hand, the source and range of the constructed CAP_project/ModulePresentationsForCAP/gap/ModulePresentationObject.gi Lines 234 to 241 in ab7d994 CAP_project/FreydCategoriesForCAP/gap/FreydCategory.gi Lines 346 to 353 in ab7d994 I don't understand why these |
|
As discussed here are the changes I made some time ago in https://github.com/homalg-project/IntrinsicModules/blob/master/gap/FreydCategory.gi This code relies on more CAP operations introduced here: https://github.com/homalg-project/CategoryConstructor/blob/master/gap/Tools.gi#L159-L237 |
TKuh
force-pushed
the
module_pres_compiled
branch
2 times, most recently
from
dac16f3 to
8612370
Compare
|
Some remarks after adding logic templates:
But other than that, this should be ready for a first review. |
|
Some points:
Sorry, I don't understand this comment. With "the change of the tensor product" you mean using the Freyd version of the tensor product (compiled), right? If you also compile the closed structure from Freyd, this is certainly compatible with the Freyd tensor product, assuming that the Freyd implementation and the logic templates are correct. The only thing we should check for performance reasons is how the manual implementation of
Do those changes disappear when compiling without the logic templates? If yes, you can bisect your logic templates to find the offending one.
|
Yes.
Ok, great!
Compiled version: gap> ZZ := HomalgRingOfIntegers();;
gap> rows := CategoryOfRows( ZZ );;
gap> lpres := LeftPresentations( ZZ );;
gap> mors := List([1 .. 6], i -> AsLeftPresentation( MorphismDatum( RandomMorphism( rows, 5 ) ) ) );;
gap> EnableTimingStatistics( lpres );;
gap> for pair in Tuples( [1..6], 2 ) do EvaluationMorphism( mors[pair[1]], mors[pair[2]] ); od;
gap> for pair in Tuples( [1..6], 2 ) do CoevaluationMorphism( mors[pair[1]], mors[pair[2]] ); od;
gap> DisplayTimingStatistics( lpres );
####
Timing statistics for the primitive operations of the category Category of left presentations of Z:
Total time spent in primitive operations of this category: 43671 ms
CoevaluationMorphismWithGivenRange was called 36 times with a total runtime of 7620 ms ( =
211666 μs per execution)
EvaluationMorphismWithGivenSource was called 36 times with a total runtime of 1587 ms ( = 44083 μs per execution)
InternalHomOnObjects was called 72 times with a total runtime of 34441 ms ( = 478347 μs per execution)
TensorProductOnObjects was called 72 times with a total runtime of 23 ms ( = 319 μs per execution)Manual version: gap> ZZ := HomalgRingOfIntegers();;
gap> rows := CategoryOfRows( ZZ );;
gap> lpres := LeftPresentations( ZZ );;
gap>
gap> mors := List([1 .. 6], i -> AsLeftPresentation( MorphismDatum( RandomMorphism( rows, 5 ) ) ) );;
gap> EnableTimingStatistics( lpres );;
gap>
gap> for pair in Tuples( [1..6], 2 ) do EvaluationMorphism( mors[pair[1]], mors[pair[2]] ); od;
gap> for pair in Tuples( [1..6], 2 ) do CoevaluationMorphism( mors[pair[1]], mors[pair[2]] ); od;
gap>
gap> DisplayTimingStatistics( lpres );
####
Timing statistics for the primWithoutitive operations of the category Category of left presentations of Z:
Total time spent in primitive operations of this category: 16715 ms
BraidingWithGivenTensorProducts was called 72 times with a total runtime of 24 ms ( = 333 μs per execution)
CoevaluationMorphismWithGivenRange was called 36 times with a total runtime of 1010 ms ( =
28055 μs per execution)
EvaluationMorphismWithGivenSource was called 36 times with a total runtime of 108 ms ( = 3000 μs per execution)
IdentityMorphism was called 42 times with a total runtime of 14 ms ( = 333 μs per execution)
InternalHomOnObjects was called 72 times with a total runtime of 7292 ms ( = 101277 μs per execution)
IsEqualForObjects was called 72 times with a total runtime of 10 ms ( = 138 μs per execution)
KernelEmbedding was called 73 times with a total runtime of 7181 ms ( = 98369 μs per execution)
LiftAlongMonomorphism was called 36 times with a total runtime of 920 ms ( = 25555 μs per execution)
PreCompose was called 108 times with a total runtime of 20 ms ( = 185 μs per execution)
TensorProductOnMorphismsWithGivenTensorProducts was called 36 times with a total runtime of 7 ms ( =
194 μs per execution)
TensorProductOnObjects was called 399 times with a total runtime of 129 ms ( = 323 μs per execution)So the manually written ones are much faster. :( The morphisms were the same in both runs.
Disabling my logic templates revealed, that it might be worse without. testing: /home/tom/.gap/pkg/CAP_project/ModulePresentationsForCAP/tst/modulepresentationsforcap04.tst
########> Diff in /home/tom/.gap/pkg/CAP_project/ModulePresentationsForCAP/tst/modulepresentationsforcap04.ts\
t:43
# Input is:
Display( l );
# Expected output:
[ [ 2406, 0 ],
[ 171, 0 ] ]
A morphism in Category of left presentations of Z
# But found:
[ [ 236554, 0 ],
[ 78415, 0 ] ]
A morphism in Category of left presentations of Z
########
########> Diff in /home/tom/.gap/pkg/CAP_project/CAP/tst/cap16.tst:46
# Input is:
Display( UnderlyingMatrix( morphism_l1 ) );
# Expected output:
[ [ -84 ],
[ -25 ] ]
# But found:
[ [ -84 ],
[ -196 ] ]
########
########> Diff in /home/tom/.gap/pkg/CAP_project/CAP/tst/cap16.tst:53
# Input is:
Display( UnderlyingMatrix( morphism_l2 ) );
# Expected output:
[ [ -39 ],
[ -55 ] ]
# But found:
[ [ -39 ],
[ -91 ] ]
########
########> Diff in /home/tom/.gap/pkg/CAP_project/CAP/tst/cap16.tst:94
# Input is:
Display( UnderlyingMatrix( morphism_r1 ) );
# Expected output:
[ [ -84, -25 ] ]
# But found:
[ [ -84, -196 ] ]
########
########> Diff in /home/tom/.gap/pkg/CAP_project/CAP/tst/cap16.tst:100
# Input is:
Display( UnderlyingMatrix( morphism_r2 ) );
# Expected output:
[ [ -39 ],
[ -55 ] ]
# But found:
[ [ -39 ],
[ -91 ] ]
########So maybe I was frightened for nothing and there might be nothing wrong with the logic templates.
No, it's an artifact.
Yes, I get a lot of warnings and an error otherwise: WARNING: Could not find declaration of INTERNAL_HOM_EMBEDDING (current input: [ <Category "IsFreydCategory">, <Cat\
egory "IsFreydCategoryObject">, <Category "IsFreydCategoryObject"> ])
while compiling function with name "Function added to Freyd( Rows( Q ) ) for InternalHomOnObjects"
located at /home/tom/.gap/pkg/CAP_project/FreydCategoriesForCAP/gap/FreydCategory.gi:1534
WARNING: Could not find declaration of INTERNAL_HOM_EMBEDDING (current input: [ <Category "IsFreydCategory">, <Cat\
egory "IsFreydCategoryObject">, <Category "IsFreydCategoryObject"> ])
while compiling function with name "Function added to Freyd( Rows( Q ) ) for InternalHomOnMorphismsWithGivenIntern\
alHoms"
located at /home/tom/.gap/pkg/CAP_project/FreydCategoriesForCAP/gap/FreydCategory.gi:1576
WARNING: Could not find declaration of INTERNAL_HOM_EMBEDDING (current input: [ <Category "IsFreydCategory">, <Cat\
egory "IsFreydCategoryObject">, <Category "IsFreydCategoryObject"> ])
while compiling function with name "Derivation (first added to Rows( Q )) of InternalHomOnMorphisms"
located at stream:2
WARNING: Could not find declaration of INTERNAL_HOM_EMBEDDING (current input: [ <Category "IsFreydCategory">, <Cat\
egory "IsFreydCategoryObject">, <Category "IsFreydCategoryObject"> ])
while compiling function with name "Function added to Freyd( Rows( Q ) ) for CoevaluationMorphismWithGivenRange"
located at /home/tom/.gap/pkg/CAP_project/FreydCategoriesForCAP/gap/FreydCategory.gi:1682
WARNING: Could not find declaration of INTERNAL_HOM_EMBEDDING (current input: [ <Category "IsFreydCategory">, <Cat\
egory "IsFreydCategoryObject">, <Category "IsFreydCategoryObject"> ])
while compiling function with name "Function added to Category of left presentations of Q for CoevaluationMorphism\
WithGivenRange"
located at stream:2
Error, Could not get rid of all global variables, see <function_string>. You should use category_hints.category_attribute\
_names. at /home/tom/.gap/pkg/CAP_project/CompilerForCAP/gap/PrecompileCategory.gi:168 called from
CapJitPrecompileCategory( category_constructor, given_arguments, package_name, compiled_category_name
); at /home/tom/.gap/pkg/CAP_project/CompilerForCAP/gap/PrecompileCategory.gi:362 called from
CapJitPrecompileCategoryAndCompareResult( function ( ring )
return LeftPresentationsAsFreydCategoryOfCategoryOfRows( ring );
end, [ ring ], "ModulePresentationsForCAP", Concatenation( "LeftPresentationsAsFreydCategoryOfCategoryOfRowsOf"
, name, "Precompiled" )
); at examples/PrecompileModulePresentationsAsFreydCategoryOfCategoryOfRowsOrColumns.g:82 called from
<function "precompile_LeftPresentations">( <arguments> )
called from read-eval loop at examples/PrecompileModulePresentationsAsFreydCategoryOfCategoryOfRowsOrColumns.g:86
you can 'quit;' to quit to outer loop, or
you can 'return;' to continue
brk> Huge thank you! |
Your fears are very sensible: Logic templates should not change results, otherwise it is very hard to judge whether the CAP operations are still consistent (and indeed, usually they are not, see for example #1194 (comment)). So that the logic templates seem to "improve" the result is not a good thing :/ Can you find out which logic templates are responsible for these changes? I suspect (at least) some of the ones including All other points are better discussed verbally, I think. Thanks for the tests! |
TKuh
force-pushed
the
module_pres_compiled
branch
from
8612370 to
f98750d
Compare
I have now tested
and in all three cases the tests return the same results and numbers. :) I think the problem in #932 (comment) was, that while I compiled with all logic templates, I didn't delete the manual code from So it does not seem, that the logic templates change results (at least for the tests). |
|
For future reference: I thought the compiled code of In any case, the use of So to continue: Please extract the compilation of |
TKuh
force-pushed
the
module_pres_compiled
branch
from
f98750d to
475f690
Compare
in ModulePresentationsForCAP
in ModulePresenationsForCAP and adjust examples
TKuh
force-pushed
the
module_pres_compiled
branch
from
475f690 to
c545f4a
Compare
|
As just discussed:
We can meet again after 1. is finished (or if there are problems, of course), 2. - 5. can come afterwards. Thanks a lot! |
For now, these are only the low-hanging fruits.
Some remarks:
AddBraidingfor right presentations inverts the permutation matrix, while the original code inverted the permutation directly before creating a matrix out of it.TensorProductOnObjectsis compilable but the compiled verison makes the exampleProjectivityTest.gfail.PreComposehas special cases eliminated when compiled, hence I did not include it for now.IsEqualForObjectshas an additional check for the dimensions of the matrices, when compiled. Hence I did not include it for now.These are easy to discuss and to add. They also don't impact the tests.
Everything else is very different when compiled. Especially the various lifts and the internal homs are complicated.