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

add 'LinearCombinationOfMorphisms' to the method record #1408

Merged
merged 3 commits into from
Aug 14, 2023

Conversation

kamalsaleh
Copy link
Member

@kamalsaleh kamalsaleh commented Aug 2, 2023

@mohamed-barakat @zickgraf
An implementation of what we discussed yesterday.

@codecov
Copy link

codecov bot commented Aug 2, 2023

Codecov Report

Patch coverage: 84.48% and no project coverage change.

Comparison is base (479ac4d) 80.63% compared to head (25ae038) 80.63%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1408   +/-   ##
=======================================
  Coverage   80.63%   80.63%           
=======================================
  Files         492      492           
  Lines       62516    62562   +46     
=======================================
+ Hits        50407    50446   +39     
- Misses      12109    12116    +7     
Flag Coverage Δ
ActionsForCAP 64.76% <ø> (ø)
AttributeCategoryForCAP 88.91% <ø> (ø)
CAP 83.90% <90.47%> (+0.02%) ⬆️
CartesianCategories 93.50% <ø> (ø)
CompilerForCAP 94.25% <ø> (ø)
ComplexesAndFilteredObjectsForCAP 73.68% <ø> (ø)
FreydCategoriesForCAP 81.25% <100.00%> (-0.01%) ⬇️
GeneralizedMorphismsForCAP 61.40% <ø> (ø)
GradedModulePresentationsForCAP 44.59% <ø> (ø)
GroupRepresentationsForCAP 72.06% <ø> (ø)
HomologicalAlgebraForCAP 73.21% <ø> (ø)
InternalExteriorAlgebraForCAP 93.08% <ø> (ø)
LinearAlgebraForCAP 66.53% <50.00%> (-0.06%) ⬇️
ModulePresentationsForCAP 68.41% <ø> (ø)
ModulesOverLocalRingsForCAP 90.70% <ø> (ø)
MonoidalCategories 89.00% <ø> (ø)
ToricSheaves 21.79% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
...recompiled_categories/MatrixCategoryPrecompiled.gi 65.87% <37.50%> (-0.07%) ⬇️
CAP/gap/MethodRecord.gi 91.94% <82.60%> (-0.06%) ⬇️
CAP/PackageInfo.g 100.00% <100.00%> (ø)
CAP/gap/AddFunctions.autogen.gd 100.00% <100.00%> (ø)
CAP/gap/CategoryMorphisms.gd 100.00% <100.00%> (ø)
CAP/gap/DerivedMethods.gi 82.13% <100.00%> (+0.05%) ⬆️
CAP/gap/OppositeCategory.gi 95.63% <100.00%> (+0.36%) ⬆️
FreydCategoriesForCAP/PackageInfo.g 100.00% <100.00%> (ø)
FreydCategoriesForCAP/gap/CategoryOfRows.gi 94.02% <100.00%> (-0.02%) ⬇️
LinearAlgebraForCAP/PackageInfo.g 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mohamed-barakat
mohamed-barakat previously approved these changes Aug 2, 2023
@kamalsaleh
Copy link
Member Author

@mohamed-barakat Fabian and I think LinearCombinationOfMorphisms would be more consistent with the current naming conventions, e.g., like SumOfMorphisms.

@mohamed-barakat
Copy link
Member

@mohamed-barakat Fabian and I think LinearCombinationOfMorphisms would be more consistent with the current naming conventions, e.g., like SumOfMorphisms.

Yes, good idea.

@kamalsaleh kamalsaleh changed the title add 'LinearCombination' to the method record add 'LinearCombinationOfMorphisms' to the method record Aug 2, 2023
CAP/gap/MethodRecord.gi Outdated Show resolved Hide resolved
@kamalsaleh kamalsaleh force-pushed the lincomb branch 3 times, most recently from 98dcb0b to 2b58213 Compare August 7, 2023 13:34
@zickgraf
Copy link
Member

zickgraf commented Aug 11, 2023

I have some thoughts which I would like to discuss in the context of this PR (although this does not necessarily block the PR):

Currently, LinearCombinationOfMorphisms is derived from SumOfMorphisms and MultiplyWithElementOfCommutativeRing, but not the other way round. This makes LinearCombinationOfMorphisms look like a convenience operation. This situation also occurs at other places: For example, MorphismBetweenDirectSums and ComponentOfMorphismInto/FromDirectSum are derived from other operations for direct sums but not the other way round.

From an implementation standpoint this makes sense: Implementing SumOfMorphisms (or even only AdditionForMorphisms) and MultiplyWithElementOfCommutativeRing is easier and much less error prone than implementing LinearCombinationOfMorphisms directly. However, from a mathematical standpoint and in the context of ur-algorithms, I think we usually want to use the operation with more semantics, that is, LinearCombinationOfMorphisms, whose semantics is being the inverse operation for BasisOfExternalHom and CoefficientsOfMorphisms. So I think theoretically it would be nice to derive SumOfMorphisms and Multiply... from LinearCombinationOfMorphisms. But then again I would never recommend implementing LinearCombinationOfMorphisms directly. Hence, I'm not sure what the best way forward is.

Background information: The context in which I first came across this question is showing that AdditiveClosure is indeed additive using CompilerForCAP. Using "higher-level" operations like MorphismBetweenDirectSums make the proof shorter, but usually no one would implement MorphismBetweenDirectSums directly.

@mohamed-barakat
Copy link
Member

Please fix the commit-message of the first commit.

'list_of_elements_of_commutative_ring_of_linear_structure' while adding methods to the opposite category
and bump CAP to V2023.08-07 and LinearAlgebraForCAP to V2023.08-02
…hisms' operation)

and bump FreydCategoriesForCAP to V2023.08-02
@zickgraf
Copy link
Member

As just discussed:

  1. In the future, the ur-algorithms should be the central building blocks, i.e. everything should be derived from the ur-algorithms.
  2. We might want to introduce "LinearCombinationOfBasisMorphisms" in the future.

@zickgraf zickgraf merged commit 83500f6 into homalg-project:master Aug 14, 2023
4 checks passed
@mohamed-barakat
Copy link
Member

Was about to write please do not merge yet :)

@zickgraf
Copy link
Member

Was about to write please do not merge yet :)

Sorry, I thought you wanted to address all effects of the CAP changes in CategoricalTowers at once.

@mohamed-barakat
Copy link
Member

Was about to write please do not merge yet :)

Sorry, I thought you wanted to address all effects of the CAP changes in CategoricalTowers at once.

No problem, now I will :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants