Skip to content

Add limited pi generator#33

Open
telser wants to merge 2 commits into
prolaag:masterfrom
telser:telser-kabbage
Open

Add limited pi generator#33
telser wants to merge 2 commits into
prolaag:masterfrom
telser:telser-kabbage

Conversation

@telser

@telser telser commented Feb 24, 2016

Copy link
Copy Markdown

Some notes:

  • It has been a while since I've used Ruby beyond just a script in a shell, so I'm not sure why the rake dependency was needed, but I couldn't get the code without my changes to work without it.
  • Digits of pi are calculated using the BBP formula https://en.wikipedia.org/wiki/Bailey%E2%80%93Borwein%E2%80%93Plouffe_formula
  • For some reason there seems to be a float overflow around 10^18, even though the max float as documented at http://ruby-doc.org/core-2.2.0/Float.html is around 1.7 x 10^308. Again, been a while since I've done Ruby, so I'm not certain what is going on here. Even if it wasn't a float, I thought Ruby automatically converts to bignums.
  • Given the above the performance, in terms of 'randomness', is actually pretty bad. Any advice?

Comment thread lib/generators/pi.rb Outdated

def next_chunk
@i +=1
x = bbp(@i).to_s.chars[0]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Using to_s.chars[0] here will only work in Ruby 2.0 or later. Instead, you can do the following, which is also compatible with older stable versions of Ruby:
to_s[0]

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I made that change.

@warmer

warmer commented Feb 25, 2016

Copy link
Copy Markdown
Collaborator

@telser - there are a couple of implementation issues in this PR.

The first, as you note, is that the algorithm fails after just a few hundred digits. After reading more about the BBP formula from the link you provided, it appears that one of the main benefits to that algorithm is that large, custom data types are not required when implemented properly, so the float range should not be an issue.

The second issue is that the actual calculation doesn't appear correct - the hex representation of the output doesn't match what I've found for hex representations of pi. Further, the output is decimal-encoded hex, and it should be binary, so there are only 4 bits of variability in every byte of output.

These issues would need to be corrected before this PR could be accepted and merged.

@telser

telser commented Mar 3, 2016

Copy link
Copy Markdown
Author

@warmer I fixed the float overflow issue, checked the first 10ish digits and another 10ish starting around 1k and 10k digits in (calling the bbp fn with 1000 and 10000).

Two further notes:

  • This is slow, it doesn't use a ton of memory, but there are quicker pi calculation methods that switching out for might be better.
  • The string directive to use in pack, is that the correct one?

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