Introduce `Sqrt` on `NumberType` #3055

darkdrag00nv2 · 2024-01-28T10:50:25Z

Work towards #1151

Description

Introduce Sqrt on NumberType defined in a Math contract.

Sqrt<T: NumberType>(value : T) : UFix64

Panics with UnderflowError if value < 0.
Panics with OverflowError if the result does not fit in a UFix64.

Targeted PR against master branch
Linked to Github issue with discussion and accepted design OR link to spec that describes this work
Code follows the standards mentioned here
Updated relevant documentation
Re-reviewed Files changed in the Github PR explorer
Added appropriate labels

codecov · 2024-01-28T11:00:08Z

Codecov Report

Attention: Patch coverage is 75.59055% with 31 lines in your changes are missing coverage. Please review.

Project coverage is 80.68%. Comparing base (a676d8f) to head (b2f799b).
Report is 495 commits behind head on master.

Files	Patch %	Lines
runtime/interpreter/value.go	38.09%	26 Missing ⚠️
runtime/interpreter/big.go	90.32%	2 Missing and 1 partial ⚠️
runtime/stdlib/math.go	96.22%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3055      +/-   ##
==========================================
+ Coverage   80.59%   80.68%   +0.08%     
==========================================
  Files         379      380       +1     
  Lines       91159    91659     +500     
==========================================
+ Hits        73472    73957     +485     
+ Misses      15053    15052       -1     
- Partials     2634     2650      +16

Flag	Coverage Δ
unittests	`80.68% <75.59%> (+0.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

darkdrag00nv2 · 2024-01-28T11:11:44Z

Haven't added any tests yet. Opened it up for some early feedback.

Are we okay with making this a top level function in the standard library?
Does the implementation look correct? Mainly the BigIntSqrt function in the runtime/interpreter/big.go file.
Should we return a Optional and return Nil in case of overflows.

turbolent · 2024-01-29T21:36:50Z

Nice work!

Are we okay with making this a top level function in the standard library?

Maybe we can introduce a Math library/contract? (similar to existing RLP, BLS, Crypto, etc.; similar to how there's a "math" package in some languages).
Alternatively we can also define it as a function on all Number types, as .squareRoot() (how e.g. Swift does it: https://developer.apple.com/documentation/swift/double/squareroot())

darkdrag00nv2 · 2024-02-01T20:11:54Z

@turbolent I like both the ideas.

@SupunS @dsainati1 Do you have opinions? I'll go with a separate math contract if both seem fine to you all.

dsainati1 · 2024-02-01T20:26:25Z

A Math contract seems the most appropriate to me

runtime/interpreter/big.go

AlexHentschel

I am less familiar with fixed point numbers, so my confusion maybe it is just my lack of context. Nevertheless, it might be useful to add some documentation making it easier for other people reading to code to follow, even if they are not experts in fixed-point number arithmetics. Please see my suggestions above (specifically this and this comment)

Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>

darkdrag00nv2 · 2024-02-16T15:59:10Z

Thanks for the review comments @AlexHentschel. Have incorporated your comments. PTAL again, thanks!

AlexHentschel

👏

tarakby

Great work and nice documentation 👌🏼
I left a few comments below.

tarakby · 2024-04-10T19:12:05Z

runtime/interpreter/big.go

+
+func BigIntSqrt(interpreter *Interpreter, value *big.Int, locationRange LocationRange) UFix64Value {
+	if value.Sign() < 0 {
+		panic(UnderflowError{


Unlike the overflow case where the result can't fit into the target type, this case corresponds to an "undefined" case rather than an underflow. The square root being non defined for negative inputs (similar to the division by zero error where dividing by zero isn't mathematically defined).

There doesn't seem to be an "undefined" error available so I see why underflow was used, but I wanted to mention this.

tarakby · 2024-04-11T03:59:29Z

runtime/interpreter/big.go

+	// This is because of the above check with sema.MaxSquareIntegerBig.
+	valueFloat := new(big.Float).SetPrec(256).SetInt(value)
+	res := new(big.Float).SetPrec(256).SetMode(big.ToZero).Sqrt(valueFloat)
+	res.Mul(res, new(big.Float).SetPrec(256).SetInt(sema.Fix64FactorBig))


new(big.Float).SetPrec(256).SetInt(Fix64FactorBig) could be a global constant defined like Fix64FactorBig.

tarakby · 2024-04-11T04:29:00Z

runtime/interpreter/value.go

+func (v Fix64Value) Sqrt(interpreter *Interpreter, locationRange LocationRange) UFix64Value {
+	val := new(big.Int).SetUint64(uint64(v))
+	sqrtWithFix64FactorSqrt := BigIntSqrt(interpreter, val, locationRange)
+	sqrt := sqrtWithFix64FactorSqrt / fixedpoint.Fix64FactorSqrt


Possible optimization: I think we can save this division if in the function BigIntSqrt (L163), we multiply by Fix64FactorSqrt instead of Fix64Factor, only in the case of Fix64Value and UFix64Value. For the rest of the types, we can keep multiplying by Fix64Factor.

tarakby · 2024-04-11T18:02:15Z

runtime/sema/type.go

+	// So we can support Sqrt till 34028236692093846346337 since
+	// Sqrt(34028236692093846346337) = 184467440737.09551615999875115311
+	// which gets rounded down to 184467440737.09551615
+	// Sqrt(34028236692093846346338) = 184467440737.09551616000146165854


tarakby · 2024-04-11T18:19:01Z

runtime/interpreter/big.go

+		})
+	}
+
+	if value.Cmp(sema.MaxSquareIntegerBig) == 1 {


When calling this check in the case of Fix64Value and UFix64Value, I think we would exclude some values because value is larger than the max, but in reality it represents a number less than max (because value is multiplied by the factor). Actually, I think all Fix64 and UFix64 values are less than the max, so the result (square root) can always fit.

If I didn't miss anything and my comment makes sense, it may be better to have a specific function BigIntSqrtFloat to process the case of UFix64 and Fix64 skipping the overflow (also to add the optimization from my previous comment)

Introduce Sqrt on NumberType

ee3813e

Define Math package and implement tests

90b485f

darkdrag00nv2 marked this pull request as ready for review February 5, 2024 14:45

darkdrag00nv2 requested review from turbolent, SupunS and dsainati1 as code owners February 5, 2024 14:45

AlexHentschel reviewed Feb 7, 2024

View reviewed changes

runtime/interpreter/big.go Outdated Show resolved Hide resolved

AlexHentschel reviewed Feb 7, 2024

View reviewed changes

runtime/interpreter/big.go Show resolved Hide resolved

runtime/interpreter/big.go Show resolved Hide resolved

runtime/interpreter/big.go Outdated Show resolved Hide resolved

runtime/interpreter/big.go Outdated Show resolved Hide resolved

AlexHentschel reviewed Feb 7, 2024

View reviewed changes

darkdrag00nv2 and others added 3 commits February 16, 2024 20:53

Add comment for the rounding mode

370e29a

Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>

Move calculation out of valueGetter

2beb223

Add comment about 256 bit precision being sufficient

b2f799b

darkdrag00nv2 requested a review from AlexHentschel February 16, 2024 15:59

AlexHentschel approved these changes Feb 20, 2024

View reviewed changes

tarakby self-requested a review April 8, 2024 22:50

tarakby reviewed Apr 11, 2024

View reviewed changes

SupunS added the Feature label Apr 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `Sqrt` on `NumberType` #3055

Introduce `Sqrt` on `NumberType` #3055

darkdrag00nv2 commented Jan 28, 2024 •

edited

codecov bot commented Jan 28, 2024 •

edited

darkdrag00nv2 commented Jan 28, 2024

turbolent commented Jan 29, 2024

darkdrag00nv2 commented Feb 1, 2024

dsainati1 commented Feb 1, 2024

AlexHentschel left a comment •

edited

darkdrag00nv2 commented Feb 16, 2024

AlexHentschel left a comment

tarakby left a comment

tarakby Apr 10, 2024

tarakby Apr 11, 2024

tarakby Apr 11, 2024

tarakby Apr 11, 2024

tarakby Apr 11, 2024

Introduce Sqrt on NumberType #3055

Are you sure you want to change the base?

Introduce Sqrt on NumberType #3055

Conversation

darkdrag00nv2 commented Jan 28, 2024 • edited

Description

codecov bot commented Jan 28, 2024 • edited

Codecov Report

darkdrag00nv2 commented Jan 28, 2024

turbolent commented Jan 29, 2024

darkdrag00nv2 commented Feb 1, 2024

dsainati1 commented Feb 1, 2024

AlexHentschel left a comment • edited

Choose a reason for hiding this comment

darkdrag00nv2 commented Feb 16, 2024

AlexHentschel left a comment

Choose a reason for hiding this comment

tarakby left a comment

Choose a reason for hiding this comment

tarakby Apr 10, 2024

Choose a reason for hiding this comment

tarakby Apr 11, 2024

Choose a reason for hiding this comment

tarakby Apr 11, 2024

Choose a reason for hiding this comment

tarakby Apr 11, 2024

Choose a reason for hiding this comment

tarakby Apr 11, 2024

Choose a reason for hiding this comment

Introduce `Sqrt` on `NumberType` #3055

Introduce `Sqrt` on `NumberType` #3055

darkdrag00nv2 commented Jan 28, 2024 •

edited

codecov bot commented Jan 28, 2024 •

edited

AlexHentschel left a comment •

edited