Repository navigation
Conversation
The kernel function behind `IsSubsetSet`, and so behind `IsSubset` for
two internal lists, merged the two sets, scanning the first one up to
the last element of the second. It now finds each element of the second
set in the first by galloping and binary search, with
O(len2 * log(len1 / len2)) comparisons; equal sets still cost one
comparison per element.
Ranges were copied into plain lists first; for `[-2^50 .. 2^50]` this
ran out of memory. Membership in a range is now decided arithmetically,
and whether a range lies in another one in constant time.
Timings with builds from the default configure options.
Setup:
gap> L1:= List( [ 1 .. 10^5 ], i -> 2*i );;
gap> L2:= [ 199996, 200000 ];;
gap> L3:= ShallowCopy( L1 );;
gap> R:= [ 2, 4 .. 2*10^5 ];;
Before:
gap> for i in [ 1 .. 10^4 ] do IsSubset( L1, L2 ); od; time;
990
gap> IsSubset( [ 1 .. 10^7 ], [ 5 ] ); time;
true
120
gap> for i in [ 1 .. 100 ] do IsSubset( R, L3 ); od; time;
99
gap> for i in [ 1 .. 1000 ] do IsSubset( L1, L3 ); od; time;
66
After:
gap> for i in [ 1 .. 10^4 ] do IsSubset( L1, L2 ); od; time;
2
gap> IsSubset( [ 1 .. 10^7 ], [ 5 ] ); time;
true
0
gap> for i in [ 1 .. 100 ] do IsSubset( R, L3 ); od; time;
27
gap> for i in [ 1 .. 1000 ] do IsSubset( L1, L3 ); od; time;
62
Fixes #771
Assisted-by: Generative AI
fingolfin
added
kind: enhancement
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6793 +/- ##
==========================================
+ Coverage 79.72% 79.77% +0.05%
==========================================
Files 684 684
Lines 295592 295632 +40
Branches 8660 8672 +12
==========================================
+ Hits 235650 235842 +192
+ Misses 58126 57975 -151
+ Partials 1816 1815 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
james-d-mitchell
left a comment
james-d-mitchell
left a comment
There was a problem hiding this comment.
Looks good to me, subject to the included comments.
| ** that is not smaller: probe i1, i1+1, i1+3, i1+7, ... until one is, then | ||
| ** search between the last two probes. This needs O(len2 * log(len1/len2)) |
There was a problem hiding this comment.
| ** that is not smaller: probe i1, i1+1, i1+3, i1+7, ... until one is, then | |
| ** search between the last two probes. This needs O(len2 * log(len1/len2)) | |
| ** that is not smaller: probe i1, i1+1, i1+3, i1+7, ... until <set1> at that position is not smaller than the given element of <set2>, then | |
| ** perform a binary search between the last two probed entries of <set1>. This needs O(len2 * log(len1/len2)) |
I had some trouble parsing this at first, some minor edits (that mess up the line length, sorry!)
There was a problem hiding this comment.
Might also be good to mention "galloping" here for the (like me) uninitiated.
There was a problem hiding this comment.
Reworded and named the technique (galloping) in the header.
| ** comparisons, and one per entry for equal sets. | ||
| */ | ||
| static BOOL IsSubsetPlainSet(Obj set1, Obj set2) | ||
| { |
There was a problem hiding this comment.
Maybe add some assertions here that set1 and set2 are indeed plain sets.
There was a problem hiding this comment.
Added GAP_ASSERT(IsPlainSet(...)) for both arguments.
| if (!LT(e1, e2)) | ||
| return FALSE; | ||
|
|
||
| // invariant: set1[lo] < e2, and set1[hi] >= e2 or hi = len1 + 1 |
There was a problem hiding this comment.
Is this comment really accurate? When lo, step, and hi are defined in the next 3 lines, this "invariant" doesn't necessary hold. Wouldn't it be more accurate to say "Find values of lo and hi such that ..."?
There was a problem hiding this comment.
Right, it only holds from the binary search on; moved it there.
The comment in IsSubsetPlainSet stated an invariant before it held and did not name the technique; both arguments are now asserted to be plain sets. Ranges with ends beyond 2^28 cannot be formed on 32-bit, so the tests with 2^50 run on 64-bit only; smaller ranges test the same code on both. Assisted-by: Generative AI
The kernel function behind
IsSubsetSet, and so behindIsSubsetfor two internal lists, merged the two sets, scanning the first one up to the last element of the second. It now finds each element of the second set in the first by galloping and binary search, with O(len2 * log(len1 / len2)) comparisons; equal sets still cost one comparison per element. Sorted lists that are not plain, such as ranges, are no longer copied: for[-2^50 .. 2^50]the copy ran out of memory.Timings with builds from the default configure options.
Setup:
Before:
After:
Fixes #771
Assisted-by: Generative AI