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.

Sign up for GitHub

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

Jump to bottom

Monotonic square root #5543

Merged
merged 10 commits into from
Jun 17, 2023
Merged

Monotonic square root #5543

merged 10 commits into from
Jun 17, 2023

Conversation

ValarDragon
Copy link
Member

@ValarDragon ValarDragon commented Jun 15, 2023 •
edited
Loading

What is the purpose of the change

Start a monotonic square root function, which aims to give the following guarantees:

  • if y = sqrt(x) then y*y >= x
  • if x1 >= x2, then sqrt(x1) >= sqrt(x2)

Testing and Verifying

This comes with tests ensuring:

  • Handles square root property within an accuracy of 1, always on correct side
  • Handles monotonicity
  • Code comments explaining why monotonicity holds

Comes with a benchmark as well, its 17x faster than what we had before, after our SDK-side optimizations.

BenchmarkMcSqrt-16    	    2509	    452016 ns/op	  277150 B/op	    4853 allocs/op
BenchmarkSqrt-16    	     154	   7542845 ns/op	 1799492 B/op	   39402 allocs/op

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes?
  • Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

  • Specification (x/{module}/README.md)
  • Osmosis documentation site
  • Code comments?
  • N/A

@ValarDragon ValarDragon added V:state/compatible/no_backport State machine compatible PR, depends on prior breaks A:no-changelog labels Jun 15, 2023
AlpinYukseloglu
AlpinYukseloglu reviewed Jun 16, 2023
View reviewed changes
osmomath/sqrt.go Outdated Show resolved Hide resolved
@ValarDragon ValarDragon changed the title start monotonic_sqrt Monotonic square root Jun 16, 2023
@ValarDragon ValarDragon marked this pull request as ready for review June 16, 2023 19:19
p0mvn
p0mvn approved these changes Jun 16, 2023
View reviewed changes
Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

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

LGTM

osmomath/sqrt.go Outdated Show resolved Hide resolved
osmomath/sqrt_test.go Outdated Show resolved Hide resolved
osmomath/sqrt_test.go Outdated Show resolved Hide resolved
@ValarDragon ValarDragon merged commit 0612bf4 into main Jun 17, 2023
@ValarDragon ValarDragon deleted the dev/monotonic_sqrt branch June 17, 2023 00:28
@ValarDragon ValarDragon mentioned this pull request Jun 17, 2023
Closed
@p0mvn p0mvn mentioned this pull request Jun 19, 2023
Closed
@AlpinYukseloglu AlpinYukseloglu mentioned this pull request Jun 21, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlpinYukseloglu AlpinYukseloglu AlpinYukseloglu left review comments

@p0mvn p0mvn p0mvn approved these changes

Assignees
No one assigned
Labels
A:no-changelog V:state/compatible/no_backport State machine compatible PR, depends on prior breaks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ValarDragon @p0mvn @AlpinYukseloglu