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.

Sign up for GitHub

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

Jump to bottom

Clean up #272

Merged
merged 4 commits into from
Jul 1, 2023
Merged

Clean up #272

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
43057c9
coding style
mohamed-barakat Jul 1, 2023
4c2dec7
relax timing comparison in AdditiveClosureOfAlgebroid_vs_QuiverRows.tst
mohamed-barakat Jul 1, 2023
e0439e1
switch to CreateCapCategoryObject/MorphismWithAttributes Object/Morph…
mohamed-barakat Jul 1, 2023
eb42997
deleted obsolete code in CategoryOfColimitQuivers
mohamed-barakat Jun 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Algebroids/tst/AdditiveClosureOfAlgebroid_vs_QuiverRows.tst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ gap> runtime_quiver := Runtime( ) - start;;
#
gap> if runtime >= runtime_quiver * 3 / 10 then Display( true ); else Display( runtime ); Display( runtime_quiver ); fi;
true
gap> if runtime <= runtime_quiver * 4 / 10 then Display( true ); else Display( runtime ); Display( runtime_quiver ); fi;
gap> if runtime <= runtime_quiver * 5 / 10 then Display( true ); else Display( runtime ); Display( runtime_quiver ); fi;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is needed, this means that the compiled code has gotten slower compared to the non-compiled code. Which commit is responsible for this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can only reproduce this on my laptop, where the noncompiled code seems to be faster than in the CI.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this always been the case, i.e. already when this test was introduced?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is new.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will wait for your answer before I merge.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awkward enough, the regression is caused by PR homalg-project/HigherHomologicalAlgebra#158:

Being on #271

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you post runtime and runtime_quiver before and after the regression?

Copy link
Member Author

@mohamed-barakat mohamed-barakat Jul 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without regression:

AdditiveClosureOfAlgebroid_vs_QuiverRows
msecs: 3367
gap> runtime;
647
gap> runtime_quiver;
1647
gap> Float( runtime_quiver * 4 / 10 );
658.8

With regression:

AdditiveClosureOfAlgebroid_vs_QuiverRows
msecs: 3316
gap> runtime;
666
gap> runtime_quiver;
1586
gap> Float( runtime_quiver * 4 / 10 );
634.4

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, my first numbers were wrong, now they are fixed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You see from the timings that with PR homalg-project/HigherHomologicalAlgebra#158 the runtime of QuiverRows is dropping.

true

#
Expand Down