-
-
Notifications
You must be signed in to change notification settings - Fork 80
Fix plural form for intword and improve performance
#273
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #273 +/- ##
=======================================
Coverage 99.51% 99.51%
=======================================
Files 11 11
Lines 818 820 +2
=======================================
+ Hits 814 816 +2
Misses 4 4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
hugovk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
- Improved code readability and performance
Yes, much nicer using bisect 👍
main:
❯ python -m timeit -n 10000 -s "from humanize import intword" 'for a in (1_000_000, 3_500_000, 1_000_000_000, 1_200_000_000, 1_000_000_000_000, 6_700_000_000_000, "1_000", "12_400", "12_490", "1_000_000", "-1_000_000", "1_200_000", "1_290_000", "999_999_999", "1_000_000_000", "-1_000_000_000", "2_000_000_000", "999_999_999_999", "1_000_000_000_000", "6_000_000_000_000", "-6_000_000_000_000", "999_999_999_999_999", "1_000_000_000_000_000", "1_300_000_000_000_000", "-1_300_000_000_000_000", "3_500_000_000_000_000_000_000", "8_100_000_000_000_000_000_000_000_000_000_000", "-8_100_000_000_000_000_000_000_000_000_000_000", "-8.1 quintilliards", 1_000_000_000_000_000_000_000_000_000_000_000_000, 1_100_000_000_000_000_000_000_000_000_000_000_000, 2_100_000_000_000_000_000_000_000_000_000_000_000): intword(a)'
Found existing alias for "python". You should use: "p"
10000 loops, best of 5: 42 usec per loopPR@
❯ python -m timeit -n 10000 -s "from humanize import intword" 'for a in (1_000_000, 3_500_000, 1_000_000_000, 1_200_000_000, 1_000_000_000_000, 6_700_000_000_000, "1_000", "12_400", "12_490", "1_000_000", "-1_000_000", "1_200_000", "1_290_000", "999_999_999", "1_000_000_000", "-1_000_000_000", "2_000_000_000", "999_999_999_999", "1_000_000_000_000", "6_000_000_000_000", "-6_000_000_000_000", "999_999_999_999_999", "1_000_000_000_000_000", "1_300_000_000_000_000", "-1_300_000_000_000_000", "3_500_000_000_000_000_000_000", "8_100_000_000_000_000_000_000_000_000_000_000", "-8_100_000_000_000_000_000_000_000_000_000_000", "-8.1 quintilliards", 1_000_000_000_000_000_000_000_000_000_000_000_000, 1_100_000_000_000_000_000_000_000_000_000_000_000, 2_100_000_000_000_000_000_000_000_000_000_000_000): intword(a)'
Found existing alias for "python". You should use: "p"
10000 loops, best of 5: 35.3 usec per loop
---
This also
src/humanize/number.py
Outdated
| if not largest_ordinal and rounded_value * power == powers[ordinal + 1]: | ||
| # After rounding, we end up just at the next power | ||
| ordinal += 1 | ||
| power = powers[ordinal] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't used:
| power = powers[ordinal] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a lot of refactoring. Thanks for noticing that this was still here. You're 100% right. It's not even needed anymore. :)
src/humanize/number.py
Outdated
|
|
||
| singular, plural = human_powers[ordinal] | ||
| unit = _ngettext(singular, plural, math.ceil(rounded_value)) | ||
| return f"{negative_prefix}{rounded_value} {unit}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this introduces a regression?
Before:
>>> intword(1234567, "%.0f")
1 millionPR:
>>> intword(1234567, "%.0f")
1.0 millionTry this:
| return f"{negative_prefix}{rounded_value} {unit}" | |
| return f"{negative_prefix}{format % rounded_value} {unit}" |
And we could add some test cases like this to the main test_intword():
(["1234567", "%.0f"], "1 million"),
(["1234567", "%.1f"], "1.2 million"),
(["1234567", "%.2f"], "1.23 million"),
(["1234567", "%.3f"], "1.235 million"),
(["999500", "%.0f"], "1 million"),
(["999499", "%.0f"], "999 thousand"),There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good catch! I added what you suggested. Thank you!
Remove unused code.
intword and improve performance
|
Thanks again! |
Fixes #175
Changes proposed in this pull request: