Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

If you try to loop through senses first, hypernym_paths() hangs #157

Closed
fcbond opened this issue Jan 3, 2022 · 5 comments
Closed

If you try to loop through senses first, hypernym_paths() hangs #157

fcbond opened this issue Jan 3, 2022 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@fcbond
Copy link
Collaborator

fcbond commented Jan 3, 2022

Hi,

if you try to loop through senses, then try to find hypernym paths, the code hangs. Looping through synsets is fine!

For example, the code below will print the path for the synset in the first loop, but hang in the second loop.

##
## MWP
##

import wn
en = wn.Wordnet('omw-en:1.4')

organism = en.synsets(ili='i35563')[0] #beast

for ss in en.synsets(pos='n'):
    if ss == organism:
        print(ss.hypernym_paths())
                
for s in en.senses(pos='n'):
    ss == s.synset()
    if ss == organism:
        print(ss.hypernym_paths())
@goodmami goodmami added the bug Something isn't working label Jan 3, 2022
@goodmami
Copy link
Owner

goodmami commented Jan 3, 2022

This is odd. I suspect the Sense.synset() method is not creating a synset object in the same way as via the Wordnet.synsets() method, and then the hypernym_paths() method attempts to expand through all installed wordnets. If so, then it should be an easy fix.

edit: here's a shorter MWE:

import wn
en = wn.Wordnet('omw-en:1.4')
en.sense('omw-en-beast-00015388-n').synset().hypernym_paths()

@goodmami
Copy link
Owner

goodmami commented Jan 3, 2022

Looks like my guess was correct. Here is Sense.synset():

wn/wn/_core.py

Lines 896 to 905 in a832a63

def synset(self) -> Synset:
"""Return the synset of the sense.
Example:
>>> wn.senses('spigot')[0].synset()
Synset('pwn-03325088-n')
"""
return synset(id=self._synset_id)

Note that it calls the module function wn.synset(), without any limits on the lexicons. The resulting, unconstrained synset will then use any installed lexicon to look for expanded relations on subsequent operations. It would be better to construct a Synset like how Wordnet.synset() does:

wn/wn/_core.py

Lines 1156 to 1162 in a832a63

def synset(self, id: str) -> Synset:
"""Return the first synset in this wordnet with identifier *id*."""
iterable = find_synsets(id=id, lexicon_rowids=self._lexicon_ids)
try:
return Synset(*next(iterable), self)
except StopIteration:
raise wn.Error(f'no such synset: {id}')

But the lexicon_rowids constraint on the call to find_synsets would need to be filled out. If a Wordnet object was used to get the sense, it would just be self._wordnet._lexicon_ids, but not if the query was done in "default mode" (which I thought was documented, but now I don't see it in the docs; see #92 (comment)). Luckily there's a non-public convenience method for this:

wn/wn/_core.py

Lines 266 to 274 in a832a63

def _get_lexicon_ids(self) -> Tuple[int, ...]:
if self._wordnet._default_mode:
return tuple(
{self._lexid}
| set(get_lexicon_extension_bases(self._lexid))
| set(get_lexicon_extensions(self._lexid))
)
else:
return self._wordnet._lexicon_ids

Secondly, the wordnet object of the sense (accessible via the _wordnet attribute) needs to be passed in the Synset constructor instead of just self.

Finally, it looks like Sense.word() has the same issue.

I'm going to assign this to you, @fcbond. If that's ok, please submit a PR and I'll look it over. If not, I can do it.

@goodmami
Copy link
Owner

@fcbond just checking in. Do you think you can do this, or shall I?

@fcbond
Copy link
Collaborator Author

fcbond commented Jan 21, 2022 via email

@goodmami
Copy link
Owner

The synset issue was apparently resolved in #170. Sense.word() still has the same problem, though.

goodmami added a commit that referenced this issue Sep 29, 2022
Fix #157: Sense.word() object uses same Wordnet
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants