4
\$\begingroup\$

I'm working on a program that aim to take sentences (currently in french) and compact them to a length of 38 characters while retaining as much information as possible.
You can find another part of this project here replacing words with their abbreviations

I've made a function that removes determiners from a string using an external config file containing said determiners. It is functionnal but I think there's a lot to improve.

Here it is:

# determiners is the config file below parsed by configparser, l1 is the string
def remove_determiners(determiners, l1):
    for key in determiners:
        it_l1 = 0

        # the goal is to obtain a string shorter than 38 char 
        while(it_l1 < len(l1) and len(l1) > 38):

             # finds possible beginnig of words
             if(l1[it_l1] in ' -' or it_l1 == 0):
                 if(it_l1 == 0):
                      it_l1 = -1

                 # finds possible end of words
                 it_word = 0
                 while(it_word < len(key) and it_l1 + it_word + 1 < len(l1)
                 and l1[it_l1 + it_word + 1] == key[it_word].upper()):
                      it_word += 1
                 if(it_word == len(key) and (it_l1 + it_word + 1 == len(l1)
                 or l1[it_l1 + it_word + 1] in ' -.')):
                      if(it_l1 == -1):
                          it_l1 = 0

                      # cuts the determiner out
                      l1 = l1[:it_l1] + l1[it_l1 + it_word + 1:]

             it_l1 += 1
             if(it_l1 == 0):
                 it_l1 = 1
    return(l1)

Here is the configfile: (mostly french determiners)

[remove]
& = nope
A = nope
AND = nope
AU = nope
AUX = nope
D = nope
DE = nope
DES = nope
DU = nope
EN = nope
FOR = nope
ET = nope
L = nope
LA = nope
LE = nope
LEUR = nope
LEURS = nope
LES = nope
OF = nope
OU = nope
PAR = nope
POUR = nope
SA = nope
SON = nope
SUR = nope
THE = nope
UN = nope
UNE = nope

Here is how its parsed:

determiners = configparser.ConfigParser()
determiners.read('configuration//determiners.ini')

And here are some I/O examples:

'JE SUIS LA BAGUETTE.' --> 'JE SUIS BAGUETTE.'
'LES PILES DE LA TELECOMMANDE.' --> 'PILES TELECOMMANDE.'
'QU ELLE HEURE EST IL ?' --> 'QU ELLE HEURE EST IL ?'

You may note the strings have been formatted to remove special characters and to be all upcase

\$\endgroup\$
  • 1
    \$\begingroup\$ Hello! From the code, it is hard to tell how the config example can be used to test the code. Could a provide one (or more) examples of inputs/outputs for remove_determiners ? \$\endgroup\$ – Josay Nov 28 '18 at 10:07
  • \$\begingroup\$ @Josay This has been done \$\endgroup\$ – Comte_Zero Nov 28 '18 at 10:20
  • \$\begingroup\$ I'm trying to run the examples you've provided with no success. Here is what I've tried: pastebin.com/JcZ9qvAS . Please make sure that you provide enough information to be able to try your code easily. \$\endgroup\$ – Josay Nov 28 '18 at 11:25
  • \$\begingroup\$ Out of curiosity why 38 character long? \$\endgroup\$ – IEatBagels Nov 29 '18 at 14:04
  • \$\begingroup\$ @IEatBagels French postal addresses can't be longer than 38 characters \$\endgroup\$ – Comte_Zero Nov 29 '18 at 14:08
5
\$\begingroup\$

Thanks for sharing your code.

It's a nice project you have there.

Naming

You should take some time to choose carefully your variable. l1 is not obvious, maybe input, input_string, sentence or text would be more appropriate?

Without the commentary above the function, it would have been hard to find what that was supposed to be.

Same for it_l1 and it_word. After reading your code a few time, I am not sure what they are. Find a descriptive name, it helps tremendously when reading code.

Documentation

You have comments, that's good. But it is way better to have a good docstring for the function, and fewer comments.

Your docstring should describe what the function try to accomplish, why it exists, the parameter (eventually their type) and what does the function return.

And more annoying, some of your comments seem to be outdated/wrong?

  • # determiners is the config file below parsed by configparser but it seems determiners is not a config file, but a list of string. (True it is the content of a file parsed by configparser, but it's not the same)
  • # the goal is to obtain a string shorter than 38 char you did change your code but did not change that comment I presume? I couldn't find where it was supposed to restrict the size to 38char

Indentation

Your code here:

while(it_word < len(key) and it_l1 + it_word + 1 < len(l1)
and l1[it_l1 + it_word + 1] == key[it_word].upper()):
    it_word += 1
if(it_word == len(key) and (it_l1 + it_word + 1 == len(l1)
or l1[it_l1 + it_word + 1] in ' -.')):

is very hard to read. Maybe it's because you pasted it into CodeReview, but be carefull when writing long and hard condition. The more difficult it is to read, the more difficult it is to debug/maintain/extend.

Putting it altogether

Note: it_l1 and it_word are still in the code because I don't know how to name them properly but you should probably change them.

def remove_determiners(determiners, text):
    """
    Removes determiners words from text

    :param determiners: the words to remove in `text`
    :param text: a text to remove determiner in
    :type determiners: List[str]
    :type text: str:
    :return: The value of `text` after removing all words present in determiners
    """
    for key in determiners:
        it_l1 = 0

        while(it_l1 < len(text)):

             # Finds possible beginning of words
             if(text[it_l1] in ' -' or it_l1 == 0):
                 if(it_l1 == 0):
                      it_l1 = -1

                 # Finds possible end of words
                 it_word = 0
                 while(it_word < len(key) 
                       and it_l1 + it_word + 1 < len(l1)
                       and text[it_l1 + it_word + 1] == key[it_word].upper()):
                      it_word += 1
                 if(it_word == len(key)
                    and (it_l1 + it_word + 1 == len(text)
                         or text[it_l1 + it_word + 1] in ' -.')):
                      if(it_l1 == -1):
                          it_l1 = 0

                      # cuts the determiner out
                      text = l1[:it_l1] + l1[it_l1 + it_word + 1:]

             it_l1 += 1
             if(it_l1 == 0):
                 it_l1 = 1
    return(text)

Algorithm

You could also really simplify your code by using set.

Below is a code that works for the simple case, need to be improved to deal with punctuation. (if you have a text like "LE, LA, ET.", it will not remove the determiners. Or use a clean up step as shown below)

def remove_determiners(determiners, text):
    """
    Removes determiners words from text

    :param determiners: the words to remove in `text`
    :param text: a text to remove determiner in
    :type determiners: List[str]
    :type text: str:
    :return: The value of `text` after removing all words present in determiners
    """

    determiners_set = set(determiners)

    text_list = text.split(' ')

    resultat = []
    for element in text_list:
        if(element not in determiners_set):
            resultat.append(element)

    return ' '.join(resultat)

Or as a two-liner using list comprehension as @Josay pointed out in the comment:

def remove_determiners(determiners, text):
    determiners_set = set(determiners)
    return ' '.join(e for e in text.split(' ') if e not in determiners_set)

Usage:

determiners = ['AND',
               'THE',
               'ET',
               'LE',
               'LA',
               'LES',
               'DE']

text = 'LES PILES DE LA TELECOMMANDE'

remove_determiners(determiners, text)
# returns 'PILES TELECOMMANDE'

Sidenote

Why in you config file you have & = nope instead of &? Is there more values than nope?

EDIT

Code changed slightly since my answer, adding the 38 char constraint. Should be straightforward to modify my response to accomodate this change

EDIT2

Also to address @Josay point about noise in the text, a cleaning step is possible like this:

def clean_text(text)
    """Remove ponctuation from text and replace it by a empty char
       for -_,.?!:; and '" by a space
       :param text: The text to remove punctuation in
       :return: Text cleaned up
    """
    text = re.sub('[-_,.?!:;]', '', text)
    text = re.sub('[\'"]', ' ', text)
    return text

And usage

determiners = ['AND',
               'THE',
               'ET',
               'LE',
               'LA',
               'LES',
               'DE']

text = 'LES PILES DE-LA TELECOMMANDE'

remove_determiners(determiners, clean_text(text))
\$\endgroup\$
  • \$\begingroup\$ nope is a filler value for the config file, there is no other value. Can I remove it ? \$\endgroup\$ – Comte_Zero Nov 28 '18 at 15:17
  • 4
    \$\begingroup\$ Excellent answer! Please note that the function behavior is slightly different from the original one in situation such as "PILES DE-LA TELE" (which makes no sense in French but it is not the point). Also, you could use a list comprehension or generator expression to write something like: return ' '.join(e for e in text.split(' ') if e not in determiners_set). \$\endgroup\$ – Josay Nov 28 '18 at 15:20
  • \$\begingroup\$ I put a note saying it behave a bit differently with punctuation, but you are right it also behave differently with if there are any char not a space between words. Also thanks for the one liner, I'm not used enough to list comprehension \$\endgroup\$ – Julien Rousé Nov 28 '18 at 15:36
  • \$\begingroup\$ @Comte_Zero I don't know if you can, but maybe try to find out? It is really redondant and useless so if you can remove it without breaking your whole project, I think you should. \$\endgroup\$ – Julien Rousé Nov 28 '18 at 15:38
  • \$\begingroup\$ I unfortunately can't \$\endgroup\$ – Comte_Zero Nov 28 '18 at 15:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.