-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Fix wordnet's all_synsets() function #3078
Conversation
f067ee0
to
518a28a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added some simple tests. I also accidentally brought in 2f34b52 into this PR, so I rebased and removed some of the unnecessary commits. Apologies for this. Make sure to update locally to avoid issues if you make changes!
I had some comments and requests for changes. I hope you'll be able to have a look at them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good! Thanks for always helping maintain Wordnet in NLTK!
Fix #3077, and a few related problems in wordnet.py.
3.0
print(len(list(wn.all_synsets(pos="a"))))
18156
print(len(list(wn.all_synsets(pos="s"))))
10693
None
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.
3.1
23098
print(len(list(wn31.all_synsets(pos="a", lang="hrv"))))
1363
print(len(list(wn31.all_synsets(pos="s", lang="hrv"))))
444