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

Make code more understandable #38

Closed
wants to merge 4 commits into from
Closed

Make code more understandable #38

wants to merge 4 commits into from

Conversation

eugene-eeo
Copy link

@eugene-eeo eugene-eeo commented Jul 31, 2016

On intword - I've "fixed" it so that intword(10**101) returns '10.0 googol' instead of a string of the actual number itself. I'm not sure if this is the intended behaviour though. Also instead of doing division and comparison with huge numbers, I've changed it so that it only looks at the exponents, i.e. the result of math.log10(n).

@jmoiron
Copy link
Owner

jmoiron commented Jul 31, 2016

The googol change is great, and I think the way they're arranged is an improvement. This creates a bit of a hole in some of the translations but that can be addressed separately.

I am concerned that some of these changes produce significantly more garbage or do more work at runtime than the originals. I don't have benchmarks to prove it, but I'd imagine:

  • log10 is much heavier than comparing against pre-computed 10**6
  • adding a list and ' '.join(..) to fractional is heavier than the original

Imperfect %timeit tests:

In [2]: tentothesixth = 10**6
In [7]: x = 1000000
In [8]: timeit x < tentothesixth
10000000 loops, best of 3: 25.6 ns per loop
In [9]: timeit math.log10(x) < 6
The slowest run took 15.85 times longer than the fastest. This could mean that an intermediate result is being cached 
10000000 loops, best of 3: 120 ns per loop

log10 might be an overall improvement for large numbers but hurt small ones. I'm not sure I have any idea what the common case is. I suppose if the number is < 10**6, there's nothing to do; otherwise, there's a bunch of division to do, and math.log10 would be closer and simpler.

@eugene-eeo
Copy link
Author

eugene-eeo commented Aug 1, 2016

@jmoiron my bad for not measuring. Check out the new changes. :)

@hugovk
Copy link
Collaborator

hugovk commented Aug 16, 2021

Thanks for the contribution, I'll close this five-year-old PR as it has merge conflicts and also lots of unnecessary files from the .eggs dir so it's hard to see the relevant changes. You're welcome to open a fresh PR, or new issue if you'd like to check things first.

Thanks again!

@hugovk hugovk closed this Aug 16, 2021
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.

3 participants