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

fix multiple zeros on split_thousand #33

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Laifsyn
Copy link

@Laifsyn Laifsyn commented May 27, 2024

Related issue: #11

Because the decimals weren't being truncated, while !num.is_zero() { never evaluates to 0 until the BigFloat can no longer display the super small number.

For the curious ones, here's a benchmark that could display the small performance improvement (Using Criterion)

Before fix

Num2Words to_cardinal()

EN FR FR_BE FR_CH UK
hundreds 24.46 us (✅ 1.00x) 24.99 us (✅ 1.02x slower) 24.57 us (✅ 1.00x slower) 24.46 us (✅ 1.00x slower) 25.06 us (✅ 1.02x slower)
thousands 25.90 us (✅ 1.00x) 25.84 us (✅ 1.00x faster) 26.01 us (✅ 1.00x slower) 25.95 us (✅ 1.00x slower) 26.64 us (✅ 1.03x slower)
millions 26.96 us (✅ 1.00x) 26.99 us (✅ 1.00x slower) 27.39 us (✅ 1.02x slower) 27.31 us (✅ 1.01x slower) 27.88 us (✅ 1.03x slower)
billions 28.18 us (✅ 1.00x) 28.28 us (✅ 1.00x slower) 28.38 us (✅ 1.01x slower) 28.38 us (✅ 1.01x slower) 29.05 us (✅ 1.03x slower)
trillions 29.08 us (✅ 1.00x) 29.42 us (✅ 1.01x slower) 29.37 us (✅ 1.01x slower) 29.31 us (✅ 1.01x slower) 30.27 us (✅ 1.04x slower)
quadrillions 30.67 us (✅ 1.00x) 30.89 us (✅ 1.01x slower) 31.16 us (✅ 1.02x slower) 31.88 us (✅ 1.04x slower) 31.21 us (✅ 1.02x slower)
nonillion 37.69 us (✅ 1.00x) 37.98 us (✅ 1.01x slower) 37.16 us (✅ 1.01x faster) 37.15 us (✅ 1.01x faster) 38.37 us (✅ 1.02x slower)

After the fix

Num2Words to_cardinal()

EN FR FR_BE FR_CH UK
hundreds 613.99 ns (✅ 1.00x) 574.91 ns (✅ 1.07x faster) 578.37 ns (✅ 1.06x faster) 592.72 ns (✅ 1.04x faster) 535.31 ns (✅ 1.15x faster)
thousands 1.79 us (✅ 1.00x) 1.70 us (✅ 1.05x faster) 1.70 us (✅ 1.05x faster) 1.68 us (✅ 1.07x faster) 1.68 us (✅ 1.07x faster)
millions 2.99 us (✅ 1.00x) 2.81 us (✅ 1.06x faster) 2.82 us (✅ 1.06x faster) 2.82 us (✅ 1.06x faster) 2.93 us (✅ 1.02x faster)
billions 4.12 us (✅ 1.00x) 4.02 us (✅ 1.02x faster) 4.05 us (✅ 1.02x faster) 4.00 us (✅ 1.03x faster) 3.90 us (✅ 1.06x faster)
trillions 5.12 us (✅ 1.00x) 5.23 us (✅ 1.02x slower) 5.00 us (✅ 1.02x faster) 5.21 us (✅ 1.02x slower) 5.15 us (✅ 1.01x slower)
quadrillions 6.80 us (✅ 1.00x) 6.66 us (✅ 1.02x faster) 6.36 us (✅ 1.07x faster) 6.61 us (✅ 1.03x faster) 6.19 us (✅ 1.10x faster)
nonillion 13.71 us (✅ 1.00x) 13.81 us (✅ 1.01x slower) 13.80 us (✅ 1.01x slower) 13.78 us (✅ 1.01x slower) 14.36 us (✅ 1.05x slower)

Made with criterion-table

@Laifsyn Laifsyn mentioned this pull request Jun 18, 2024
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.

1 participant