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

Simpler(?) version of numerical decomposition #182

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

Conversation

Tibo-lg
Copy link
Member

@Tibo-lg Tibo-lg commented Dec 23, 2021

This PR proposes an update to the numerical decomposition document, to try to make things a bit easier to understand. I think "easy to understand" is a subjective notion though and so if people don't think this version is simpler I'll happily close this PR.

One thing to note is that unless I'm mistaken, the stated O(B*log_B(L)) seems incorrect to me as the worst case is above it.

Also if people think rust is not easy to read I could change it to typescript. I hope the code comments are enough to understand the code though.

Copy link
Contributor

@benthecarman benthecarman left a comment

Choose a reason for hiding this comment

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

Just some nits

but I think this is easier to understand and I think rust is more approachable for most ppl and it seems a lot of the bitcoin space is moving towards rust

This optimization is called the **endpoint optimization**.
while value > 0 {
res.push(value % base);
value = ((value as f64) / (base as f64)).floor() as usize;
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe should just make value and base f64s insted of casting them since it is just an example

Copy link
Member Author

Choose a reason for hiding this comment

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

Mmmmm I'm a bit hesitant because I feel it's quite important to return integers and not floats and I'd be worried that someone just copy pasting that thinks that they can just manipulate floats all over and be fine (tbh I'm not sure if it's actually fine or not, but I think it's just better to avoid it).

startDigits.last.to(endDigits.last).toVector.map { lastDigit =>
prefixDigits :+ lastDigit

// All the single digits in ]start[0]; end[0][ are sufficient to cover their respective intervals.
Copy link
Contributor

Choose a reason for hiding this comment

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

]start[0]; end[0][

I think this is meant to be [start[0]; end[0]]

Copy link
Member Author

Choose a reason for hiding this comment

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

I think ] start[0]; end[0] [ is correct. If it were [start[0]; end[0]] then in the example it would include 1 and 4 which are covered in the loops above and under. Let me know if maybe something is unclear (or if maybe I'm just missing something).

@Tibo-lg
Copy link
Member Author

Tibo-lg commented Jan 17, 2022

but I think this is easier to understand

Happy to hear :)

@Christewart Christewart self-requested a review April 12, 2022 22:56
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