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.

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

Avoid unnecessary trigger of derivations and do not rely on AddPrimitiveOperation when installing final derivations #1690

Merged
merged 4 commits into from
Sep 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 CAP/PackageInfo.g
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ SetPackageInfo( rec(

PackageName := "CAP",
Subtitle := "Categories, Algorithms, Programming",
Version := "2024.09-23",
Version := "2024.09-24",
Date := (function ( ) if IsBound( GAPInfo.SystemEnvironment.GAP_PKG_RELEASE_DATE ) then return GAPInfo.SystemEnvironment.GAP_PKG_RELEASE_DATE; else return Concatenation( ~.Version{[ 1 .. 4 ]}, "-", ~.Version{[ 6, 7 ]}, "-01" ); fi; end)( ),
License := "GPL-2.0-or-later",

Expand Down
37 changes: 25 additions & 12 deletions CAP/gap/Derivations.gi
Original file line number Diff line number Diff line change
Expand Up @@ -108,13 +108,6 @@
function( CAP_NAMED_ARGUMENTS, d, weight, C )
local method_name, func;

Info( DerivationInfo, 1, Concatenation( "install(",
String( weight ),
") ",
TargetOperation( d ),
": ",
Description( d ), "\n" ) );

method_name := TargetOperation( d );
func := DerivationFunction( d );

Expand Down Expand Up @@ -459,14 +452,26 @@

if new_weight < current_weight or (new_weight = current_weight and current_derivation <> fail and d!.position_in_derivations_by_target < current_derivation!.position_in_derivations_by_target) then

Info( DerivationInfo, 1, Concatenation( "install(",
String( new_weight ),
") ",

Check warning on line 457 in CAP/gap/Derivations.gi

View check run for this annotation

Codecov / codecov/patch

CAP/gap/Derivations.gi#L456-L457

Added lines #L456 - L457 were not covered by tests
target,
": ",

Check warning on line 459 in CAP/gap/Derivations.gi

View check run for this annotation

Codecov / codecov/patch

CAP/gap/Derivations.gi#L459

Added line #L459 was not covered by tests
Description( d ), "\n" ) );

# Previously, `InstallDerivationForCategory` was called at this point.
# However, this could lead to methods being overwritten if cheaper derivations become available while adding primitive installations to a category.
# Hence, we now install the derivations in `Finalize`.

owl!.operation_weights.( target ) := new_weight;
owl!.operation_derivations.( target ) := d;

InstallDerivationsUsingOperation( owl, target );
# if the weight has not changed, there is no need to re-trigger the chain of derivations
if new_weight <> current_weight then

InstallDerivationsUsingOperation( owl, target );

fi;

fi;

Expand Down Expand Up @@ -519,18 +524,26 @@

InstallMethod( AddPrimitiveOperation,
[ IsOperationWeightList, IsString, IsInt ],
function( owl, op_name, weight )
function( owl, op_name, new_weight )
local current_weight;

Info( DerivationInfo, 1, Concatenation( "install(",
String( weight ),
String( new_weight ),

Check warning on line 531 in CAP/gap/Derivations.gi

View check run for this annotation

Codecov / codecov/patch

CAP/gap/Derivations.gi#L531

Added line #L531 was not covered by tests
") ",
op_name,
": primitive installation\n" ) );

owl!.operation_weights.( op_name ) := weight;
current_weight := owl!.operation_weights.( op_name );

owl!.operation_weights.( op_name ) := new_weight;
owl!.operation_derivations.( op_name ) := fail;

InstallDerivationsUsingOperation( owl, op_name );
# if the weight has not changed, there is no need to re-trigger the chain of derivations
if new_weight <> current_weight then

InstallDerivationsUsingOperation( owl, op_name );

fi;

end );

Expand Down
29 changes: 24 additions & 5 deletions CAP/gap/Finalize.gi
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@
[ "FinalizeCategory", true ],
],
function( CAP_NAMED_ARGUMENTS, category )
local derivation_list, weight_list, current_install, current_final_derivation, weight, old_weights, categorical_properties, diff, properties_with_logic, property, i, derivation, operation, property_name, installed_operations_of_homomorphism_structure, original_REORDER_METHODS_SUSPENSION_LEVEL;
local derivation_list, weight_list, current_install, current_final_derivation, op_name, new_weight, current_weight, old_weights, categorical_properties, diff, properties_with_logic, property, i, derivation, operation, property_name, installed_operations_of_homomorphism_structure, original_REORDER_METHODS_SUSPENSION_LEVEL;

if IsFinalized( category ) then

Expand Down Expand Up @@ -391,16 +391,35 @@

for derivation in current_final_derivation.derivations do

weight := OperationWeightUsingDerivation( weight_list, derivation );
op_name := TargetOperation( derivation );
new_weight := OperationWeightUsingDerivation( weight_list, derivation );
current_weight := CurrentOperationWeight( weight_list, op_name );

Assert( 0, weight <> infinity );
Assert( 0, new_weight <> infinity );

# When installing a final derivation bundle, the installation of the first operations in the bundle
# might trigger (normal) derivations of later operations it the bundle, which might be cheaper then
# the derivations provided in the bundle.
if weight <= CurrentOperationWeight( weight_list, TargetOperation( derivation ) ) then
if new_weight <= current_weight then

InstallDerivationForCategory( derivation, weight, category : IsFinalDerivation := true );
Info( DerivationInfo, 1, Concatenation( "install(",
String( new_weight ),
") ",

Check warning on line 407 in CAP/gap/Finalize.gi

View check run for this annotation

Codecov / codecov/patch

CAP/gap/Finalize.gi#L406-L407

Added lines #L406 - L407 were not covered by tests
op_name,
": ",

Check warning on line 409 in CAP/gap/Finalize.gi

View check run for this annotation

Codecov / codecov/patch

CAP/gap/Finalize.gi#L409

Added line #L409 was not covered by tests
Description( derivation ), "\n" ) );

InstallDerivationForCategory( derivation, new_weight, category : IsFinalDerivation := true );

weight_list!.operation_weights.( op_name ) := new_weight;
weight_list!.operation_derivations.( op_name ) := fail;

# if the weight has not changed, there is no need to re-trigger the chain of derivations
if new_weight <> current_weight then

InstallDerivationsUsingOperation( weight_list, op_name );

fi;

fi;

Expand Down
3 changes: 1 addition & 2 deletions CAP/gap/InstallAdds.gi
Original file line number Diff line number Diff line change
Expand Up @@ -285,9 +285,8 @@ InstallMethod( AddCapOperation,

fi;

if not is_derivation then
if not (is_derivation or is_final_derivation) then

# Final derivations are not handled by the original derivation mechanism and are thus just like primitive operations for it.
AddPrimitiveOperation( category!.derivations_weight_list, function_name, weight );

fi;
Expand Down
3 changes: 3 additions & 0 deletions FreydCategoriesForCAP/gap/CategoryOfRows.gi
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,9 @@ InstallMethod( CategoryOfRows,

SetIsRigidSymmetricCoclosedMonoidalCategory( cat, true );

# since methods have been added before, we have to reevaluate the derivations
Reevaluate( cat!.derivations_weight_list );

fi;

INSTALL_FUNCTIONS_FOR_CATEGORY_OF_ROWS( cat );
Expand Down
Loading