feat: Add Elias-Fano encoding#235
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates how small antimagmas’ multiplication tables are serialized in package metadata by switching from the old Convert/Reverse representation to a new packed integer encoding, and wires the new decode path into the public constructors.
Changes:
- Add cached helpers plus
MultiplicationTableEncode/Decode/GetEntryfor base-(n^n) packed integer encoding of multiplication tables. - Update
SmallAntimagma,AllSmallAntimagmas, andReallyAllSmallAntimagmasto decode metadata using the new encoding. - Add a dedicated test suite for the new encoding and update the packaged
.g.gzmetadata files accordingly.
Reviewed changes
Copilot reviewed 4 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
lib/helper.gd |
Adds encoding/decoding helpers and caching used to interpret metadata as packed integers. |
lib/smallantimagmas.gi |
Switches magma construction to use MultiplicationTableDecode(..., order) on metadata entries. |
lib/utils.gi |
Minor refactor to name the tuples enumerator before filtering. |
tst/test_helper_multiplication_table_encoding.tst |
New tests for caches, encode/decode round-trips, injectivity, and old/new equivalence. |
data/non-isomorphic/{2,3,4}/small_*.g.gz |
Updated non-isomorphic metadata content to match the new encoding format. |
data/all/{2,3}/small_*.g.gz |
Updated “all” metadata content to match the new encoding format. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| local row_id, powers_nn, nn, row_dict; | ||
| powers_nn := __SmallAntimagmaHelper.PowersOfNNWithCache(n); | ||
| nn := n ^ n; | ||
| row_dict := __SmallAntimagmaHelper.RowDictWithCache(n); | ||
| row_id := QuoInt(encoded, powers_nn[r]) mod nn; | ||
| return row_dict[row_id + 1][c]; |
There was a problem hiding this comment.
MultiplicationTableGetEntry is described as O(1) access, but it forces construction of the full RowDictWithCache(n) (size n^n) before returning a single entry. If this is intended for true O(1) memory access, consider computing the digit directly from encoded (and avoid RowDictWithCache), or clarify in the comment that it’s O(1) only after the row dictionary cache is built.
| local row_id, powers_nn, nn, row_dict; | |
| powers_nn := __SmallAntimagmaHelper.PowersOfNNWithCache(n); | |
| nn := n ^ n; | |
| row_dict := __SmallAntimagmaHelper.RowDictWithCache(n); | |
| row_id := QuoInt(encoded, powers_nn[r]) mod nn; | |
| return row_dict[row_id + 1][c]; | |
| local row_id, powers_nn, powers_n, nn; | |
| powers_nn := __SmallAntimagmaHelper.PowersOfNNWithCache(n); | |
| powers_n := __SmallAntimagmaHelper.PowersOfNWithCache(n); | |
| nn := n ^ n; | |
| row_id := QuoInt(encoded, powers_nn[r]) mod nn; | |
| return QuoInt(row_id, powers_n[c]) mod n + 1; |
| nn := n ^ n; | ||
| powers_n := | ||
| __SmallAntimagmaHelper.PowersOfNWithCache(n); | ||
| cache[n] := List([0 .. nn - 1], i -> | ||
| List([1 .. n], | ||
| c -> (QuoInt(i, powers_n[c]) mod n) + 1)); | ||
| MakeImmutable(cache[n]); |
There was a problem hiding this comment.
RowDictWithCache materializes and caches all n^n possible rows. For n even moderately large this grows extremely fast and can exhaust memory (e.g., n=8 creates 16,777,216 rows). Consider avoiding full precomputation (compute a row on demand from row_id), or guard/limit caching to small n where it’s known to be safe.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
|
||
| # ==================================================================== | ||
| # Encode a multiplication table (list of lists, entries 1..n) to a | ||
| # single immediate integer using base-n^n packing. |
There was a problem hiding this comment.
The comment claims the encoding returns a "single immediate integer", but for larger n the packed value can exceed GAP’s immediate integer range and become a big integer. Either qualify this (e.g., "immediate for small n") or drop "immediate" to keep the documentation accurate.
| # single immediate integer using base-n^n packing. | |
| # single integer using base-n^n packing. |
| __SmallAntimagmaHelper.MultiplicationTableDecode := | ||
| function(encoded, n) | ||
| local powers_nn, nn, row_dict; | ||
| powers_nn := __SmallAntimagmaHelper.PowersOfNNWithCache(n); | ||
| nn := n ^ n; | ||
| row_dict := __SmallAntimagmaHelper.RowDictWithCache(n); | ||
| return List([1 .. n], function(r) | ||
| local row_id; | ||
| row_id := QuoInt(encoded, powers_nn[r]) mod nn; | ||
| return row_dict[row_id + 1]; |
There was a problem hiding this comment.
MultiplicationTableDecode currently uses ... mod nn, which silently maps any integer (including negative/out-of-range values) to some table. This is more permissive than the previous Reverse-based decoding and can mask corrupted/invalid metadata. Consider validating that encoded is a non-negative integer and within the expected range (0..(n^n)^n-1), or document explicitly that decoding is performed modulo (n^n)^n.
* Fix spelling: "ambigous" → "ambiguous" in error messages Co-authored-by: limakzi <50334623+limakzi@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: limakzi <50334623+limakzi@users.noreply.github.com>
No description provided.