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

improve pronouns change (perspective) #48

Open
wants to merge 3 commits into
base: 21.02
Choose a base branch
from

Conversation

emphasize
Copy link

@emphasize emphasize commented Nov 30, 2021

... by leveraging the skills translate_namedvalues to be able to fine grain the vocabulary changed

Problem

The method used limits the possibilities in several ways:

  • it is pretty rare in different languages that my and our would equate to your with changing the perspective
  • grammar isn't taken into account (german: mein, meine, meinen, but also in many other langs)
  • further additions have to be made within the code itself

Thus i think that using translate_namedvalues is the better approach to change the pronouns. The reminder is iterated over and changed as the vocabulary is found.

This also reverts changes made introducing My.dialog, Our.dialog and Your.dialog replacing those with a single Perspective.value containing the vocabulary pairs. (all available langs)

CLA

👍

Tested

👍

... by leveraging the skills translate_namedvalues to be able to fine grain the vocabulary changed
@emphasize
Copy link
Author

emphasize commented Nov 30, 2021

actually -while rarely occuring- a while loop would be better

for k, v in vocabulary.items():
    while f" {k} " in utterance:
        utterance = utterance.replace(f" {k} ", f" {v} ")

and probably reminder = f" {reminder} "

Copy link
Contributor

@krisgesling krisgesling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for helping to improve the dialog for all languages!

This generally looks good to me. Do you want to make the other little changes you suggested?

return reminder
vocabulary = self.translate_namedvalues("Perspective")

reminder = f" {reminder}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the space at the end seems like a good change. Shouldn't happen in english unless the stt is cut off but could in some other language.


reminder = f" {reminder}"
for k, v in vocabulary.items():
if f" {k} " in reminder:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the if is fine. What's the thinking behind a while?

Copy link
Author

@emphasize emphasize Dec 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the sentence is "remind me that i take my teeth out ouf my mouth". It would only change one. (Pretty rare, but consistent) - allthough that would make a heck of a sentence 🤣

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default I think String.replace() replaces all instances of the search string:

>>> reminder = " remind me that i take my teeth out ouf my mouth "
>>> reminder.replace("my", "our")
' remind me that i take our teeth out ouf our mouth '

There is an optional parameter if you want to limit that to n instances:


>>> reminder.replace("my", "our", 1)
' remind me that i take our teeth out ouf my mouth '

So if I'm not mistaken, in this case we should only need to call that once.

It is quite an interesting sentence too lol

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh, that shouldn't happen. (the mistake)

vocabulary = self.translate_namedvalues("Perspective")

reminder = f" {reminder}"
for k, v in vocabulary.items():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we try and use full variable names, just for readability and consistency.

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

Successfully merging this pull request may close these issues.

2 participants