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

outsourced a derivation of IsHomSetInhabited from Toposes to FinSetsForCAP #211

Merged
merged 1 commit into from
Jul 3, 2023
Merged

outsourced a derivation of IsHomSetInhabited from Toposes to FinSetsForCAP #211

merged 1 commit into from
Jul 3, 2023

Conversation

mohamed-barakat
Copy link
Member

No description provided.

@mohamed-barakat
Copy link
Member Author

@zickgraf: I am unable to write the correct logic template.

zickgraf
zickgraf reviewed Jun 7, 2023
View reviewed changes
gap/CompilerLogic.gi Outdated Show resolved Hide resolved
@mohamed-barakat mohamed-barakat force-pushed the IsHomSetInhabited branch 2 times, most recently from 03e6b42 to 9ff5487 Compare June 7, 2023 15:23
@codecov
Copy link

codecov bot commented Jun 7, 2023 •
edited
Loading

Codecov Report

Patch coverage: 85.00% and project coverage change: -0.16 ⚠️

Comparison is base (519f2e5) 100.00% compared to head (09746cd) 99.84%.

Additional details and impacted files 
@@             Coverage Diff             @@
##            master     #211      +/-   ##
===========================================
- Coverage   100.00%   99.84%   -0.16%     
===========================================
  Files           12       12              
  Lines         1866     1881      +15     
===========================================
+ Hits          1866     1878      +12     
- Misses           0        3       +3
Impacted Files Coverage Δ
gap/SkeletalFinSets.gi 99.38% <75.00%> (-0.62%) ⬇️
PackageInfo.g 100.00% <100.00%> (ø)
gap/CompilerLogic.gi 100.00% <100.00%> (ø)
...etalFinSetsWithMorphismsGivenByListsPrecompiled.gi 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mohamed-barakat
Copy link
Member Author

mohamed-barakat commented Jun 8, 2023 •
edited
Loading

The outsourced derivation still exists in Toposes, which is the cause for the incomplete coverage. If I remove the derivation from Toposes before this PR is merged I will get an error in the CI of CategoricalTowers.

@mohamed-barakat
Copy link
Member Author

I do not understand why the CI does not show full coverage. IsHomSetInhabited is covered in the examples.

zickgraf
zickgraf requested changes Jul 3, 2023
View reviewed changes
gap/CompilerLogic.gi Outdated Show resolved Hide resolved
gap/SkeletalFinSets.gi Outdated Show resolved Hide resolved
zickgraf
zickgraf reviewed Jul 3, 2023
View reviewed changes
gap/SkeletalFinSets.gi
@@ -985,6 +977,27 @@ end, 1 + Sum( [ [ "ExponentialOnObjects", 1 ],

end );

##
AddDerivationToCAP( IsHomSetInhabited,
"IsHomSetInhabited using IsInitial when the range category of the homomorphism structure is the skeletal category of finite sets",
Copy link
Member

Choose a reason for hiding this comment

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

General comment which does not block the PR: The information you provide in this description is nearly the same as would be displayed by DerivationsOfMethodByCategory anyway. Unfortunately, this is the case for many descriptions of derivations. It would be more useful if descriptions would describe the mathematics/semantics behind the derivations.

zickgraf
zickgraf approved these changes Jul 3, 2023
View reviewed changes
Copy link
Member

@zickgraf zickgraf left a comment

Choose a reason for hiding this comment

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

Feel free to merge the PR yourself in coordination with a suitable PR to CategoricalTowers. Please check the code coverage after all PRs are merged.

@mohamed-barakat mohamed-barakat merged commit 15c3eda into homalg-project:master Jul 3, 2023
2 of 4 checks passed
@mohamed-barakat mohamed-barakat deleted the IsHomSetInhabited branch July 3, 2023 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zickgraf zickgraf zickgraf approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mohamed-barakat @zickgraf