Skip to content

AddToIgs improvements 4: avoid recalculating IGSValFun - #238

Merged
fingolfin merged 1 commit into
gap-packages:masterfrom
stertooy:igs-improv-4
Oct 17, 2026
Merged

fingolfin merged 1 commit into
gap-packages:masterfrom
stertooy:igs-improv-4

Conversation

@stertooy

Copy link
Copy Markdown
Collaborator

Extracted from #174, builds upon #235. I'll mark this a draft PR until that one is merged and this has been rebased.

This only really does one thing, which is ensuring IGSValFun is not recalculated when we already know its value.

Original PR disclaimer:
Assisted-by: Claude Code (Opus 5.5, Fable 5.1)

@codecov

codecov Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.64%. Comparing base (b4845dc) to head (f25e0a2).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #238      +/-   ##
==========================================
+ Coverage   69.58%   69.64%   +0.05%     
==========================================
  Files          88       88              
  Lines       12875    12899      +24     
==========================================
+ Hits         8959     8983      +24     
  Misses       3916     3916              
Files with missing lines Coverage Δ
gap/basic/pcppara.gi 93.27% <100.00%> (+0.34%) ⬆️
gap/basic/pcppcps.gi 83.80% <100.00%> (+0.37%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@fingolfin fingolfin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This PR is marked as "draft" so I am not merging it.

Something else: perhaps one of these PRs should add a comment to CHANGES.md that indicates a speed up in Igs computation? Plus fixes...

@stertooy

Copy link
Copy Markdown
Collaborator Author

I'll mark this as ready once I get it rebased :)

Something else: perhaps one of these PRs should add a comment to CHANGES.md that indicates a speed up in Igs computation? Plus fixes...

Yes, currently we list AddToIgs (and variants) in the big list of fixed functions. And originally it often a bit slower than the old implementation. But I'm quite confident that with all these improvements it will be faster overall, and in many cases much faster. So with that, and given the importance of calculating igs'es, it probably deserves its own separate entry in the changelog, somewhere near the top.

@stertooy
stertooy marked this pull request as ready for review October 17, 2026 11:28
@stertooy

Copy link
Copy Markdown
Collaborator Author

Rebased and ready for review :)

@fingolfin
fingolfin merged commit e426dee into gap-packages:master Oct 17, 2026
5 checks passed
@stertooy
stertooy deleted the igs-improv-4 branch October 21, 2026 22:41
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.

2 participants