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

Function is_known() not working as expected (missing word when a rule is active) #125

Closed
adbar opened this issue May 14, 2024 · 4 comments · Fixed by #128
Closed

Function is_known() not working as expected (missing word when a rule is active) #125

adbar opened this issue May 14, 2024 · 4 comments · Fixed by #128
Labels
question Further information is requested

Comments

@adbar
Copy link
Owner

adbar commented May 14, 2024

Summary

The case happens when a word can be decomposed without being in the dictionary (here because of a rule).

from simplemma import lemmatize, is_known

Legacy

>>> lemmatize("Bergungen", lang="de")
'Bergung'
>>> is_known("Bergungen", lang="de")
True

Current state

>>> lemmatize("Bergungen", lang="de")
'Bergung'
>>> is_known("Bergungen", lang="de")
False
>>> is_known("Bergungen", lang="de", greedy=True)
False

Trying to debug

>>> from simplemma.strategies.dictionary_lookup import DictionaryLookupStrategy
>>> d = DictionaryLookupStrategy()
>>> d.get_lemma("Bergungen", "de")
>>> d.get_lemma("Berge", "de")
'Berg'

Problem

The word "Bergungen" is in the original input list and it can be lemmatized so it is "known". It is already present in the previous package versions. The result should be comparable to "Berge", i.e. something is found.
Also, the greedy option doesn't do anything (although there are rules that can be applied here).

Suggestions:

  • Since there is a rule with the words in -ung they are not part of the dictionary file anymore to save some space. So this is an issue with the way language dictionaries are created, or we should apply rules to is_known() on top of the dictionary.
  • Get rid of greedy argument for is_known() ?

@juanjoDiaz What do you think?

@adbar adbar added the question Further information is requested label May 14, 2024
@juanjoDiaz
Copy link
Collaborator

Hi @adbar ,

The is_known function only look if the word is in dictionary.
This is how it always worked (even before the refactoring to classes).

And you are right. The greedy option can be removed as it does nothing.
But that would change as soon as we apply all the the strategies.

We have 2 options:
Option 1: Keep is_known and apply all the rules as they are.

Option 2: Remove the is_known function from the Lemmatizer class and remove the fallback_strategy protocol and it's use in the lemmatize method.
This means that, when a lemma is not found, None will be returned and the user can decide how to act on it.
At the moment, the default fallback_strategy move the word to lowercase for certain languages, although I'm not sure how necessary this is.
We can, of course, keep the legacy is_known function.

I like the latter, as it simplifies things.
However, I'm not sure of the impact of removing the lowercasing.

What do you think?

@adbar
Copy link
Owner Author

adbar commented May 15, 2024

It turns out I actually use is_known() so it was a mistake to alter its functioning. It can be useful to know if a token is a dictionary word or not. If we just apply rules we save space but we treat nonsensical words the same as dictionary words.

Rules

The rules were first about extending coverage, there are not meant to save space, there are other means for that, see #72 (using binary strings instead of strings in particular). So I'd be in favor of these changes:

is_known()

Then we could keep the function but make it simpler:

  1. Simplify it further (not sure how, it's already an alias to the dictionary lookup)
  2. Remove the greedy argument since it has no effect in this context

@juanjoDiaz Would you be interested in implementing part of these changes?

And by the way to you want/need write access to the repository? I already added you as co-author in the text since the sum of your contribution is highly significant.

Maybe you also want to work on the documentation which I recently (finally) made available.

@juanjoDiaz
Copy link
Collaborator

It turns out I actually use is_known() so it was a mistake to alter its functioning. It can be useful to know if a token is a dictionary word or not. If we just apply rules we save space but we treat nonsensical words the same as dictionary words.

Makes sense.

However, the more I think about it, the more I think that we should remove the is_known function from the class and just leave the helper function calling the dictionary lookup strategy directly.

Skip rules during dictionary pickling, i.e. remove this condition
simplemma/training/dictionary_pickler.py

We can definitely do that.
It would grow the dictionaries, but it would make matching of these terms faster and improve the is_knownfunction.

Save space by converting the dictionaries to bytestrings before pickling and words to bytestrings (str.encode("utf-8")) before checking the dictionaries, these steps should be enough (?)
Pickling, possibly here
simplemma/training/dictionary_pickler.py

Queries, within this function
simplemma/simplemma/strategies/dictionary_lookup.py

This sounds good.
We should do some tests to check how much is that savings.
And what the performance impact of converting every incoming token to match to bytestring to do the comparison.
Just to be sure.

is_known()
Then we could keep the function but make it simpler:

Simplify it further (not sure how, it's already an alias to the dictionary lookup)
Remove the greedy argument since it has no effect in this context

Let's do this.
I think that we should remove it from the class and keep the function just as an alias to the dictionary lookup without the greedy option.

@juanjoDiaz Would you be interested in implementing part of these changes?

Sure. I can do this once we align on the way forward.

And by the way to you want/need write access to the repository? I already added you as co-author in the text since the sum of your contribution is highly significant.

Thanks! I appreciate you giving me credit.
I probably don't need the access.
But it's probably good to have more than one person with access.
And I think that it would allow me to add a link to the docs in the repo side bar and things like that.

Maybe you also want to work on the documentation which I recently (finally) made available.

Sure thing!
I'll comment on this on the issue about the docs 🙂

@adbar
Copy link
Owner Author

adbar commented May 16, 2024

Let's work on your PR now (I just had one comment) and break the other points apart in new issue threads (changes in dictionary pickler and tests for binary string).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants