Don't compound a capitalized word that is genuinely in BÍN#25
Merged
Conversation
The case-lookup family (lookup_nominative/accusative/dative/genitive)
and lookup_forms() ran the word compounder on a capitalized surface
form whose canonical lowercase spelling is a real BÍN word. As a result
'Æðarvarp' resolved to the synthetic compound 'æðar-varp' (bin_id 0)
instead of the genuine noun 'æðarvarp' (bin_id 353172), regressing
NounPhrase declension in GreynirEngine ('Æðarvarp 17' -> 'Æðar-varp 17').
The compounder is meant to fire only when a word is genuinely absent
from BÍN. Broaden that guard in _lookup_case() and lookup_forms() to
also recognize a capitalized form whose lowercase spelling is in BÍN;
the empty result for the exact casing then lets the caller retry with
the lowercase form, as before.
Genuine compounds absent from BÍN in any casing ('Síamskattarkjóll')
and user-hyphenated words ('Vestur-hestur') still resolve via the
compounder unchanged. Add a regression test and bump to 1.3.2.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression where the case-lookup APIs and lookup_forms() could incorrectly invoke the compounder for a capitalized surface form whose lowercase spelling is present in BÍN (e.g. Æðarvarp), producing a synthetic compound instead of letting callers retry with the lowercase form.
Changes:
- Extend the “don’t compound if the word exists in BÍN” guard to also treat
w.lower()being in BÍN as “present”, for non-lowercase inputs. - Add a regression test ensuring capitalized words whose lowercase forms exist in BÍN are not compounded.
- Bump package version to 1.3.2 (and update lockfile).
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/islenska/bindb.py | Broadens the compounding guard in _lookup_case() and lookup_forms() to recognize lowercase-in-BÍN for capitalized inputs. |
| test/test_bin.py | Adds a regression test covering the Æðarvarp/æðarvarp scenario and ensuring genuine absent compounds still compound. |
| pyproject.toml | Version bump to 1.3.2. |
| uv.lock | Lockfile version bump for the editable islenska package. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+834
to
+836
| if m or bc.contains(lemma) or ( | ||
| not lemma.islower() and bc.contains(lemma.lower()) | ||
| ): |
Comment on lines
+974
to
+979
| if ( | ||
| m | ||
| or bin_id is not None | ||
| or self.contains(w) | ||
| or (not w.islower() and self.contains(w.lower())) | ||
| ): |
Comment on lines
+760
to
+763
| assert case_func("Æðarvarp") == [] | ||
| assert all( | ||
| e.bin_id != 0 and "-" not in e.bmynd for e in case_func("æðarvarp") | ||
| ) |
Comment on lines
+766
to
+770
| assert db.lookup_forms("Æðarvarp", "hk", "EF") == [] | ||
| assert all( | ||
| e.bin_id != 0 and "-" not in e.bmynd | ||
| for e in db.lookup_forms("æðarvarp", "hk", "EF") | ||
| ) |
Comment on lines
+774
to
+777
| assert not db.contains("síamskattarkjóll") | ||
| assert all( | ||
| e.bin_id == 0 for e in db.lookup_nominative("Síamskattarkjóll") | ||
| ) |
The build job assumed the prepare-cache job had populated the cache and unzipped the file directly. But GitHub's cache service is eventually consistent: on a cold cache the entry prepare-cache saves is not always queryable when the build job's restore runs right after it, so the cache misses and the later 'unzip' step fails on the missing file — even though prepare-cache is green. (Re-runs hit, since the cache is committed by then.) Mirror the prepare-cache download as a fallback so a cache miss is never fatal; the build job then saves the cache for subsequent jobs and runs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Use `w != w.lower()` instead of `not w.islower()` in the compound guard (both in _lookup_case() and lookup_forms()). This states the intent more directly — a capitalized form whose lowercase spelling differs — and, as a side effect, short-circuits before the redundant contains() lookup that `not w.islower()` would trigger for strings with no cased characters (e.g. digit/punctuation tokens). Capture the lowercase lookup results in the regression test and assert they are non-empty before checking bin_id/hyphen constraints, so the all(...) assertions can no longer pass vacuously on an empty result. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The case-lookup family (
lookup_nominative/accusative/dative/genitive) andlookup_forms()ran the word compounder on a capitalized surface form whose canonical lowercase spelling is a genuine BÍN word. As a resultÆðarvarpresolved to the synthetic compoundæðar-varp(bin_id 0) instead of the real nounæðarvarp(bin_id 353172).This regressed
NounPhrasedeclension in GreynirEngine, e.g.NounPhrase("Æðarvarp 17").nominativereturnedÆðar-varp 17instead ofÆðarvarp 17.Root cause
The compounder is meant to fire only when a word is genuinely absent from BÍN. The guard in
_lookup_case()andlookup_forms()only checkedcontains(w)with the exact casing — forÆðarvarpthat isFalse(BÍN holds the lowercaseæðarvarp), so it fell through to compounding and fabricatedæðar-varp.Fix
Broaden that guard to also recognize a capitalized form whose lowercase spelling is in BÍN. The empty result for the exact casing then lets the caller retry with the lowercase form, exactly as before — this is the path GreynirEngine's
NounPhrasealready takes.Genuine compounds absent from BÍN in any casing (
Síamskattarkjóll) and user-hyphenated words (Vestur-hestur) still resolve via the compounder unchanged. The synthetic-hyphen contract oflookup_*()(controlled byadd_compound_hyphens) is unchanged — this only stops compounding words that are real BÍN entries.Tests
test_capitalized_word_in_bin_not_compoundedas a regression test.Version bumped to 1.3.2. GreynirEngine will require
islenska>=1.3.2once this is published to PyPI.🤖 Generated with Claude Code