simplemma icon indicating copy to clipboard operation
simplemma copied to clipboard

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

Open adbar opened this issue 1 year ago • 3 comments

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 avatar May 14 '24 15:05 adbar

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?

juanjoDiaz avatar May 14 '24 19:05 juanjoDiaz

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:

  • Skip rules during dictionary pickling, i.e. remove this condition https://github.com/adbar/simplemma/blob/6049223c9d06ec32c5ec01d8e20390bb5b125e97/training/dictionary_pickler.py#L81
  • 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 (?)
    1. Pickling, possibly here https://github.com/adbar/simplemma/blob/6049223c9d06ec32c5ec01d8e20390bb5b125e97/training/dictionary_pickler.py#L118
    2. Queries, within this function https://github.com/adbar/simplemma/blob/6049223c9d06ec32c5ec01d8e20390bb5b125e97/simplemma/strategies/dictionary_lookup.py#L29

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.

adbar avatar May 15 '24 15:05 adbar

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 🙂

juanjoDiaz avatar May 15 '24 19:05 juanjoDiaz

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).

adbar avatar May 16 '24 15:05 adbar