Uh oh!
There was an error while loading. Please reload this page.
Create lz78.py - #11842
Conversation
for more information, see https://pre-commit.ci
fixed errors
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Christian Clauss <cclauss@me.com>
added more tests
fixed some errors
Ahmed99125
commented
Oct 8, 2024
@cclauss please review |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
adding error handling and more tests
Co-authored-by: Christian Clauss <cclauss@me.com>
fixed a bug in a test case
Ahmed99125
commented
Oct 8, 2024
@cclauss raised an exception when the input is not string and added the for loop in the last test case. |
Ahmed99125
commented
Oct 10, 2024
Can anyone review my work? |
cclauss
commented
Sep 4, 2026
@priya-sundaram-dev, your review, please. Is this really how the lz78 algorithm works? Is it efficient? |
priya-sundaram-dev
left a comment
There was a problem hiding this comment.
Reviewed @Ahmed99125's compression/lz78.py. Nice, readable structure — the Token dataclass and the greedy dictionary build read well. Two questions were asked, so I'll take them in turn.
Is this really how LZ78 works?
The core is correct: extend the current phrase while it stays in the dictionary, and when phrase first falls out of the dictionary emit (index(phrase[:-1]), phrase[-1]) and add the new phrase. That's textbook LZ78 (equivalent to the classic w/w+c loop).
But there's a correctness bug: the compressor is lossy whenever the input ends on a phrase that's already in the dictionary — i.e. when the final phrase never triggers the "not in dict" branch, so it's never emitted. The trailing characters are silently dropped. This isn't a rare corner case; it hits very ordinary inputs:
compress → decompress round-trip:
"aa" -> "a" (lost 1 char)
"mississippi" -> "mississipp" (lost the final "i")
"banana" -> "banan"
"aacaacabcabaaac" -> "aacaacabcabaaa" (this file's own compress() doctest input — the final "c" is lost)
The last one is worth calling out: the compress doctest uses "aacaacabcabaaac" and the decompress doctest expects "aacaacabcabaaa", so the two doctests are internally consistent but together they demonstrate the data loss rather than catch it. The __main__ example sidesteps it by using the already-truncated "aacaacabcabaaa", which happens to end on a phrase boundary.
Root cause: after the loop, a non-empty phrase is a repeat of an existing dictionary entry with no following character, and nothing flushes it. A one-line fix restores losslessness (a token whose char is empty — decompress already appends phrase_dict[index] + "" correctly, so no change needed there):
forcharintext:
...
ifphrase: # flush the trailing phrase (an existing entry, no new char)tokens.append(Token(int(phrase_dict[phrase]), ""))
returntokensI ran this fix against the examples above plus a 2000-case random fuzz over decompress(compress(s)) == s and it round-trips cleanly on all of them.
I'd also suggest the doctest carry a round-trip property rather than only all(len(s) >= len(compress(s)) ...) — the current check asserts the output isn't longer than the input, which is exactly what stayed green while data was being dropped. Something like:
>>>all(lz78_compressor.decompress(lz78_compressor.compress(s)) ==s
... forsin ("", "a", "aa", "mississippi", "banana", "ababcbababaa"))
Truewould have failed loudly on the bug and will guard against regressions.
Is it efficient?
For an educational implementation, the dict-of-phrases approach is idiomatic and fine. Two small notes, neither blocking:
- Dictionary codes are stored as
str(code)and converted back withint(...)on every emit. Keeping them as plainintremoves the round-tripping and a class of stringly-typed bugs. - The scalable data structure for LZ78 is a trie (each node = a dictionary entry), which gives O(1) amortized work per input character. The current approach rebuilds growing string keys (
phrase += char) and hashes them, so long repeated runs cost O(L) per lookup. Totally acceptable for this repo's teaching goals — worth a one-line comment noting the trie as the production structure, but not something I'd require here.
Net: solid and close, but the lossy trailing-phrase case should be fixed (plus the round-trip doctest) before merge, since a compressor that can't reproduce its input is the one thing it must not do. Thanks @Ahmed99125!
priya-sundaram-dev
commented
Sep 4, 2026
@cclauss I reviewed it. Two answers: Is it really LZ78? Yes — the structure is textbook LZ78: a phrase dictionary seeded with the empty string at index 0, greedily extending the current phrase until it's unseen, then emitting a Is it correct? No — there's a real data-loss bug, and the PR's own doctests quietly document it. Look at the decompress doctest: but the matching compress doctest feeds This is the classic LZ78 dangling-final-phrase case: when the input ends on a phrase that's already in the dictionary, the loop's The fix is a one-liner flush after the loop — emit a terminal token for the leftover phrase (it's guaranteed to be in the dict, so its char slot is empty): ...
returntokens# <- replace with:ifphrase:
tokens.append(Token(int(phrase_dict[phrase]), ""))
returntokens
Is it efficient? Fine for the repo's teaching purpose, with two caveats worth a nudge: (1) it's the hash-map-of-full-phrase-strings variant rather than a trie, so hashing the growing Net: good algorithm, right idea, but block on the correctness bug — the round-trip must be lossless before merge. |
…batch) (#15189) @cclauss managed the new-root-directory batch from #15081, closing/merging all except #11842 (data-loss bug) and #12392 (no tests). Check the boxes for the 10 now-resolved PRs across all three listings (flat, grouped-by-directory, and 'Creates a new root directory'). Merged: #9896, #9388, #12141, #13231Closed: #13172, #12140, #13119, #11574, #13924, #14611 Still open (unchanged): #11842, #12392, #12648
Describe your change:
Added LZ78 compression algorithm.
Fixes#11837
Checklist: