Repository: nltk/nltk
Pull request: Fix wordnet's all_synsets() function
Fix #3077, and a few related problems in wordnet.py.

1. With the English Wordnet, properly distinguish satellites from adjectives:

```
from nltk.corpus import wordnet as wn
print(wn.get_version())
```
3.0

`print(len(list(wn.all_synsets(pos="a"))))`
18156

`print(len(list(wn.all_synsets(pos="s"))))`
10693

2. With other languages, a part of the problem is due to the _synset_from_pos_and_offset()_ function, which returns an empty synset object when an offset is incorrect. This leads to inflated synset counts with OMW for two reasons: OMW does include some incorrect offsets, and with later wordnets some synsets that are lost in mapping should not be counted. So, when a synset does not exist, this PR returns _None_ instead of an empty synset object:

```
ss = wn.synset_from_pos_and_offset('n',1234)
print(ss)
```
None

```
wn.add_omw()
print(len(list(wn.all_synsets(lang="hrv"))))
```
23115
`print(len(list(wn.all_synsets(pos="n", lang="hrv"))))`
16177
`print(len(list(wn.all_synsets(pos="v", lang="hrv"))))`
4736


Another problem is that the handling of satellites in the mappings was inconsistent. The mapping procedure has now been rewritten in a style that makes it easier to follow the underlying algorithm.

```
from nltk.corpus import wordnet31 as wn31
print(wn31.get_version())
```
3.1
```
wn31.add_omw()
print(len(list(wn31.all_synsets(lang="hrv"))))
```
23098
`print(len(list(wn31.all_synsets(pos="a", lang="hrv"))))`
1363
`print(len(list(wn31.all_synsets(pos="s", lang="hrv"))))`
444


Problem statement:
Incorrect part-of-speech filtering in Wordnet's all_synsets()
The _all_synsets()_ function in _wordnet.py_ does not behave as expected when using the _pos_ parameter:

1. With the English wordnet, it confuses adjectives and satellites:

```
from nltk.corpus import wordnet as wn
print(wn.get_version())
```
3.0

`print(len(list(wn.all_synsets(pos="a"))))`
18156

`print(len(list(wn.all_synsets(pos="s"))))`
18156

So, in both cases it returns all the adjectives. This is ok when using pos='a', but when pos='s', the comments inside the function state that only satellites should be returned.

2. With other languages, it does no part-of-speech filtering at all:

```wn.add_omw()
print(len(list(wn.all_synsets(lang="hrv"))))
```
23120
`print(len(list(wn.all_synsets(pos="n", lang="hrv"))))`
23120
`
print(len(list(wn.all_synsets(pos="v", lang="hrv"))))`
23120

Hints from issue discussion:

Commit message: Fix all_synsets() function

Patch to review:
diff --git a/nltk/corpus/reader/wordnet.py b/nltk/corpus/reader/wordnet.py
index c19e42d1b3..9021fcd0c1 100644
--- a/nltk/corpus/reader/wordnet.py
+++ b/nltk/corpus/reader/wordnet.py
@@ -108,6 +108,11 @@
     "Somebody %s VERB-ing",
     "It %s that CLAUSE",
     "Something %s INFINITIVE",
+    # OEWN additions:
+    "Somebody %s at something",
+    "Somebody %s for something",
+    "Somebody %s on somebody",
+    "Somebody %s out of somebody",
 )
 
 SENSENUM_RE = re.compile(r"\.[\d]+\.")
@@ -481,11 +486,11 @@ def lemma_names(self, lang="eng"):
         if lang == "eng":
             return self._lemma_names
         else:
-            self._wordnet_corpus_reader._load_lang_data(lang)
-
-            i = self._wordnet_corpus_reader.ss2of(self, lang)
-            if i in self._wordnet_corpus_reader._lang_data[lang][0]:
-                return self._wordnet_corpus_reader._lang_data[lang][0][i]
+            reader = self._wordnet_corpus_reader
+            reader._load_lang_data(lang)
+            i = reader.ss2of(self)
+            if i in reader._lang_data[lang][0]:
+                return reader._lang_data[lang][0][i]
             else:
                 return []
 
@@ -1203,59 +1208,75 @@ def __init__(self, root, omw_reader):
         # load the exception file data into memory
         self._load_exception_map()
 
+        self.nomap = []
+        self.splits = {}
+
         # map from WordNet 3.0 for OMW data
         self.map30 = self.map_wn30()
 
         # Language data attributes
         self.lg_attrs = ["lemma", "none", "def", "exe"]
 
-    def corpus2sk(self, corpus=None):
+    def indexSense(self, version=None):
         """Read sense key to synset id mapping from index.sense file in corpus directory"""
         fn = "index.sense"
-        if corpus:
+        if version:
             from nltk.corpus import CorpusReader, LazyCorpusLoader
 
-            ixreader = LazyCorpusLoader(corpus, CorpusReader, r".*/" + fn)
+            ixreader = LazyCorpusLoader(version, CorpusReader, r".*/" + fn)
         else:
             ixreader = self
         with ixreader.open(fn) as fp:
-            sk_map = {}
+            sensekeyMap = {}
             for line in fp:
-                items = line.strip().split(" ")
-                sk = items[0]
-                pos = self._pos_names[int(sk.split("%")[1].split(":")[0])]
-                sk_map[sk] = f"{items[1]}-{pos}"
-        return sk_map
+                fields = line.strip().split()
+                sensekey = fields[0]
+                pos = self._pos_names[int(sensekey.split("%")[1].split(":")[0])]
+                sensekeyMap[sensekey] = f"{fields[1]}-{pos}"
+        return sensekeyMap
+
+    def mapToMany(self):
+        sensekeyMap1 = self.indexSense("wordnet")
+        sensekeyMap2 = self.indexSense()
+        synsetToMany = {}
+        for synsetid in set(sensekeyMap1.values()):
+            synsetToMany[synsetid] = []
+        for sensekey in set(sensekeyMap1.keys()).intersection(set(sensekeyMap2.keys())):
+            source = sensekeyMap1[sensekey]
+            target = sensekeyMap2[sensekey]
+            synsetToMany[source].append(target)
+        return synsetToMany
+
+    def mapToOne(self):
+        synsetToMany = self.mapToMany()
+        synsetToOne = {}
+        for source in synsetToMany.keys():
+            candidatesBag = synsetToMany[source]
+            if candidatesBag:
+                candidatesSet = set(candidatesBag)
+                if len(candidatesSet) == 1:
+                    target = candidatesBag[0]
+                else:
+                    counts = []
+                    for candidate in candidatesSet:
+                        counts.append((candidatesBag.count(candidate), candidate))
+                    self.splits[source] = counts
+                    target = max(counts)[1]
+                synsetToOne[source] = target
+                if source[-1] == "s":
+                    # Add a mapping from "a" to target for applications like omw,
+                    # where only Lithuanian and Slovak use the "s" ss_type.
+                    synsetToOne[f"{source[:-1]}a"] = target
+            else:
+                self.nomap.append(source)
+        return synsetToOne
 
     def map_wn30(self):
         """Mapping from Wordnet 3.0 to currently loaded Wordnet version"""
         if self.get_version() == "3.0":
             return None
-        # warnings.warn(f"Mapping WN v. 3.0 to Wordnet v. {self.version}")
-        sk1 = self.corpus2sk("wordnet")
-        sk2 = self.corpus2sk()
-
-        skmap = {}
-        for sk in set(sk1.keys()).intersection(set(sk2.keys())):
-            of1 = sk1[sk]
-            of2 = sk2[sk]
-            if of1 not in skmap.keys():
-                skmap[of1] = [of2]
-            else:
-                skmap[of1].append(of2)
-
-        map30 = {}
-        for of in skmap.keys():
-            candidates = skmap[of]
-            # map to candidate that covers most lemmas:
-            of2 = max((candidates.count(x), x) for x in set(candidates))[1]
-            # warnings.warn(f"Map {of} {of2}")
-            map30[of] = of2
-            if of[-1] == "s":
-                # Add a mapping from "a" to "a" for applications like omw,
-                # which don't use the "s" ss_type:
-                map30[f"{of[:-1]}a"] = f"{of2[:-1]}a"
-        return map30
+        else:
+            return self.mapToOne()
 
     # Open Multilingual WordNet functions, contributed by
     # Nasruddin A’aidil Shari, Sim Wei Ying Geraldine, and Soe Lynn
@@ -1264,13 +1285,10 @@ def of2ss(self, of):
         """take an id and return the synsets"""
         return self.synset_from_pos_and_offset(of[-1], int(of[:8]))
 
-    def ss2of(self, ss, lang=None):
+    def ss2of(self, ss):
         """return the ID of the synset"""
-        pos = ss.pos()
-        # Only these 3 WordNets retain the satellite pos tag
-        if lang not in ["nld", "lit", "slk"] and pos == "s":
-            pos = "a"
-        return f"{ss.offset():08d}-{pos}"
+        if ss:
+            return f"{ss.offset():08d}-{ss.pos()}"
 
     def _load_lang_data(self, lang):
         """load the wordnet data of the requested language from the file to
@@ -1314,6 +1332,10 @@ def add_provs(self, reader):
                     lang = f"{lang}_{prov}"
                 self.provenances[lang] = prov
 
+    def strip_prov(self, lang):
+        #        return lang[:lang.find("_")+1]
+        return lang.split("_")[0]
+
     def add_omw(self):
         self.add_provs(self._omw_reader)
         self.omw_langs = set(self.provenances.keys())
@@ -1540,7 +1562,7 @@ def synset_from_pos_and_offset(self, pos, offset):
             assert synset._offset == offset
             self._synset_offset_cache[pos][offset] = synset
         else:
-            synset = Synset(self)
+            synset = None
             warnings.warn(f"No WordNet synset found for pos={pos} at offset={offset}.")
         data_file.seek(0)
         return synset
@@ -1808,15 +1830,14 @@ def all_omw_synsets(self, pos=None, lang=None):
             return None
         self._load_lang_data(lang)
         for of in self._lang_data[lang][0].keys():
-            try:
+            if not pos or of[-1] == pos:
                 ss = self.of2ss(of)
-                yield ss
-            except:
-                # A few OMW offsets don't exist in Wordnet 3.0.
-                # Additionally, when mapped to later Wordnets,
-                # increasing numbers of synsets are lost in the mapping.
-                #    warnings.warn(f"Language {lang}: no synset found for {of}")
-                pass
+                if ss:
+                    yield ss
+
+    #            else:
+    # A few OMW offsets don't exist in Wordnet 3.0.
+    #                warnings.warn(f"Language {lang}: no synset found for {of}")
 
     def all_synsets(self, pos=None, lang="eng"):
         """Iterate over all synsets with a given part of speech tag.
@@ -1840,12 +1861,14 @@ def all_eng_synsets(self, pos=None):
         # generate all synsets for each part of speech
         for pos_tag in pos_tags:
             # Open the file for reading.  Note that we can not re-use
-            # the file poitners from self._data_file_map here, because
+            # the file pointers from self._data_file_map here, because
             # we're defining an iterator, and those file pointers might
             # be moved while we're not looking.
             if pos_tag == ADJ_SAT:
-                pos_tag = ADJ
-            fileid = "data.%s" % self._FILEMAP[pos_tag]
+                pos_file = ADJ
+            else:
+                pos_file = pos_tag
+            fileid = "data.%s" % self._FILEMAP[pos_file]
             data_file = self.open(fileid)
 
             try:
@@ -1865,12 +1888,11 @@ def all_eng_synsets(self, pos=None):
                         # adjective satellites are in the same file as
                         # adjectives so only yield the synset if it's actually
                         # a satellite
-                        if synset._pos == ADJ_SAT:
+                        if pos_tag == ADJ_SAT and synset._pos == ADJ_SAT:
                             yield synset
-
                         # for all other POS tags, yield all synsets (this means
                         # that adjectives also include adjective satellites)
-                        else:
+                        elif pos_tag != ADJ_SAT:
                             yield synset
                     offset = data_file.tell()
                     line = data_file.readline()
@@ -2191,9 +2213,20 @@ def custom_lemmas(self, tab_file, lang):
                         # Map offset_pos to current Wordnet version:
                         offset_pos = self.map30[offset_pos]
                     else:
-                        # Synsets with no mapping keep their Wordnet 3.0 offset
-                        # warnings.warn(f"No map for {offset_pos}, {lang}: {lemma}")
-                        pass
+                        # Some OMW offsets were never in Wordnet:
+                        if (
+                            offset_pos not in self.nomap
+                            and offset_pos.replace("a", "s") not in self.nomap
+                        ):
+                            warnings.warn(
+                                f"{lang}: invalid offset {offset_pos} in '{line}'"
+                            )
+                        continue
+                elif offset_pos[-1] == "a":
+                    wnss = self.of2ss(offset_pos)
+                    if wnss and wnss.pos() == "s":  # Wordnet pos is "s"
+                        # Label OMW adjective satellites back to their Wordnet pos ("s")
+                        offset_pos = self.ss2of(wnss)
                 pair = label.split(":")
                 attr = pair[-1]
                 if len(pair) == 1 or pair[0] == lg:


Code review comments:
diff_hunk: @@ -1314,6 +1332,10 @@ def add_provs(self, reader):
                     lang = f"{lang}_{prov}"
                 self.provenances[lang] = prov
 
+    def strip_prov(self, lang):
+        #        return lang[:lang.find("_")+1]
+        return lang.split("_")[0]
+
path: nltk/corpus/reader/wordnet.py
text: @user1:
Is this meant as a public method, or is it just a leftover method that is now no longer used?

@user1:
Also, the commented line can be removed.

diff_hunk: @@ -1203,59 +1208,75 @@ def __init__(self, root, omw_reader):
         # load the exception file data into memory
         self._load_exception_map()
 
+        self.nomap = []
+        self.splits = {}
+
         # map from WordNet 3.0 for OMW data
         self.map30 = self.map_wn30()
 
         # Language data attributes
         self.lg_attrs = ["lemma", "none", "def", "exe"]
 
-    def corpus2sk(self, corpus=None):
+    def indexSense(self, version=None):
path: nltk/corpus/reader/wordnet.py
text: @user1:
```suggestion
    def index_sense(self, version=None):
```
Could we stick with snake case method names, as suggested by the Python [style guide](https://peps.python.org/pep-0008/#method-names-and-instance-variables). Optionally, we can use a single underscore in front of a method name if it's not meant to be used by the public. This may be the case for e.g. `mapToMany`?

@author:
Thanks @user1, I have added all your suggestions for the function names, and also changed the variable names.

diff_hunk: @@ -1203,59 +1208,75 @@ def __init__(self, root, omw_reader):
         # load the exception file data into memory
         self._load_exception_map()
 
+        self.nomap = []
+        self.splits = {}
+
         # map from WordNet 3.0 for OMW data
         self.map30 = self.map_wn30()
 
         # Language data attributes
         self.lg_attrs = ["lemma", "none", "def", "exe"]
 
-    def corpus2sk(self, corpus=None):
+    def indexSense(self, version=None):
         """Read sense key to synset id mapping from index.sense file in corpus directory"""
         fn = "index.sense"
-        if corpus:
+        if version:
             from nltk.corpus import CorpusReader, LazyCorpusLoader
 
-            ixreader = LazyCorpusLoader(corpus, CorpusReader, r".*/" + fn)
+            ixreader = LazyCorpusLoader(version, CorpusReader, r".*/" + fn)
         else:
             ixreader = self
         with ixreader.open(fn) as fp:
-            sk_map = {}
+            sensekeyMap = {}
             for line in fp:
-                items = line.strip().split(" ")
-                sk = items[0]
-                pos = self._pos_names[int(sk.split("%")[1].split(":")[0])]
-                sk_map[sk] = f"{items[1]}-{pos}"
-        return sk_map
+                fields = line.strip().split()
+                sensekey = fields[0]
+                pos = self._pos_names[int(sensekey.split("%")[1].split(":")[0])]
+                sensekeyMap[sensekey] = f"{fields[1]}-{pos}"
+        return sensekeyMap
+
+    def mapToMany(self):
path: nltk/corpus/reader/wordnet.py
text: @user1:
```suggestion
    def map_to_many(self):
```
Idem. I'd rather also see snake casing for the variables.

@author:
Fixed, I also did the same with variable names. Thanks @user1!

diff_hunk: @@ -1203,59 +1208,75 @@ def __init__(self, root, omw_reader):
         # load the exception file data into memory
         self._load_exception_map()
 
+        self.nomap = []
+        self.splits = {}
+
         # map from WordNet 3.0 for OMW data
         self.map30 = self.map_wn30()
 
         # Language data attributes
         self.lg_attrs = ["lemma", "none", "def", "exe"]
 
-    def corpus2sk(self, corpus=None):
+    def indexSense(self, version=None):
         """Read sense key to synset id mapping from index.sense file in corpus directory"""
         fn = "index.sense"
-        if corpus:
+        if version:
             from nltk.corpus import CorpusReader, LazyCorpusLoader
 
-            ixreader = LazyCorpusLoader(corpus, CorpusReader, r".*/" + fn)
+            ixreader = LazyCorpusLoader(version, CorpusReader, r".*/" + fn)
         else:
             ixreader = self
         with ixreader.open(fn) as fp:
-            sk_map = {}
+            sensekeyMap = {}
             for line in fp:
-                items = line.strip().split(" ")
-                sk = items[0]
-                pos = self._pos_names[int(sk.split("%")[1].split(":")[0])]
-                sk_map[sk] = f"{items[1]}-{pos}"
-        return sk_map
+                fields = line.strip().split()
+                sensekey = fields[0]
+                pos = self._pos_names[int(sensekey.split("%")[1].split(":")[0])]
+                sensekeyMap[sensekey] = f"{fields[1]}-{pos}"
+        return sensekeyMap
+
+    def mapToMany(self):
+        sensekeyMap1 = self.indexSense("wordnet")
+        sensekeyMap2 = self.indexSense()
+        synsetToMany = {}
+        for synsetid in set(sensekeyMap1.values()):
+            synsetToMany[synsetid] = []
+        for sensekey in set(sensekeyMap1.keys()).intersection(set(sensekeyMap2.keys())):
+            source = sensekeyMap1[sensekey]
+            target = sensekeyMap2[sensekey]
+            synsetToMany[source].append(target)
+        return synsetToMany
+
+    def mapToOne(self):
path: nltk/corpus/reader/wordnet.py
text: @user1:
```suggestion
    def map_to_one(self):
```
Idem. I'd rather also see snake casing for the variables.