Skip to content
This repository has been archived by the owner on Sep 8, 2024. It is now read-only.

improve runtime #2402

Merged
merged 1 commit into from
Nov 29, 2019
Merged

improve runtime #2402

merged 1 commit into from
Nov 29, 2019

Conversation

maxbachmann
Copy link
Contributor

@maxbachmann maxbachmann commented Nov 28, 2019

Description

This PR improved the runtime of the pronounce_number_en function.
Depending on the arguments provided it is 2-5 times as fast, mainly by reducing the need to copy the dicts from common_data_en.py and type conversions.

How to test

The behavior stays the same and is already covered by tests

Contributor license agreement signed?

CLA [ Yes ]

@pep8speaks
Copy link

pep8speaks commented Nov 28, 2019

Hello @maxbachmann! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-11-29 15:57:09 UTC

@devs-mycroft devs-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Nov 28, 2019
@forslund
Copy link
Collaborator

This looks great,

not related to your work in this PR but I do think the docstring for the num is incorrect. It states

num(float or int): the number to pronounce (under 100)

Would you mind handling this while you're touching this code. If you don't have the time I can do it later.

@forslund forslund self-requested a review November 29, 2019 13:51
@maxbachmann
Copy link
Contributor Author

@forslund so this should probably be changed to

num(float or int): the number to pronounce

since it can be anything between -inf and +inf

@forslund
Copy link
Collaborator

That sounds good 👍

@maxbachmann
Copy link
Contributor Author

ok applied

@forslund
Copy link
Collaborator

Many thanks, Merging now.

@forslund forslund merged commit e7d9e8b into MycroftAI:dev Nov 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants