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

fix: allow constant values of infininitesimal non-zero floating points #1185

Merged
merged 5 commits into from
Oct 21, 2023

Conversation

ajnavarro
Copy link
Contributor

@ajnavarro ajnavarro commented Oct 2, 2023

Added a unit test and an integration test trying to reproduce #1150

Avoid unhandled errors.

  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@ajnavarro ajnavarro requested a review from thehowl October 2, 2023 13:20
@ajnavarro ajnavarro requested review from jaekwon, moul and a team as code owners October 2, 2023 13:20
@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Oct 2, 2023
@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (ce258b1) 47.31% compared to head (ad43409) 47.95%.
Report is 11 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1185      +/-   ##
==========================================
+ Coverage   47.31%   47.95%   +0.63%     
==========================================
  Files         367      369       +2     
  Lines       62118    62987     +869     
==========================================
+ Hits        29394    30204     +810     
- Misses      30325    30333       +8     
- Partials     2399     2450      +51     
Files Coverage Δ
gnovm/pkg/gnolang/values_conversions.go 4.65% <11.11%> (+0.86%) ⬆️

... and 20 files with indirect coverage changes

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

@ajnavarro
Copy link
Contributor Author

ajnavarro commented Oct 2, 2023

@moul having issues with codecov API :( Appears to be fixed now

@moul
Copy link
Member

moul commented Oct 2, 2023

Yep, @gfanton made a change in #1186

Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

Thank you for the fix 🙏

gnovm/pkg/gnolang/values_conversions.go Show resolved Hide resolved
gnovm/pkg/gnolang/values_conversions_test.go Show resolved Hide resolved
Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

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

I don't think the PR fixes the behaviour; or at least, the provided test does not check for the panic I encountered.

Try running gno run on this file, from master:

package main

func main() {
	SmallestNonzeroFloat64 := 0x1p-1022 * 0x1p-52 // 4.9406564584124654417656879286822137236505980e-324
	DividedByTwo := SmallestNonzeroFloat64 / 2

	println(DividedByTwo)
}

You'll notice that the result is 0, as expected. Because this is not currently problematic in Gno code: the problem is not in floating point division per se (which works correctly), but rather in relation to constants, which have to use Bigdec for their underlying representation (in this scenario, when the division occurs, both numbers in the operation are already floats). Which is why, as in the OP of #1150, it is important that we instead test them as constants:

package main

const (
	SmallestNonzeroFloat64 = 0x1p-1022 * 0x1p-52            // 4.9406564584124654417656879286822137236505980e-324
	DividedByTwo           = SmallestNonzeroFloat64 / 2
)

func main() {
	println(DividedByTwo)
}

(To be clear, the expected behaviour here, matching what Go is doing, is to have DividedByTwo == 0).

They can be declared as constants within the function if scoping was your issue, I don't think that matters

@ajnavarro
Copy link
Contributor Author

Ok, It wasn't clear at least for me that the problem was only for constants. Changing the gno testfile to see if it fails.

Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
@ajnavarro
Copy link
Contributor Author

@thehowl changed the integration test to use constants and now I was able to reproduce and fix the error.

@thehowl thehowl changed the title fix: test smallest float divided by zero fix: allow constant values of infininitesimal non-zero floating points Oct 5, 2023
@ajnavarro
Copy link
Contributor Author

@thehowl something extra needed to be done on this PR? Thx!

@thehowl
Copy link
Member

thehowl commented Oct 12, 2023

@ajnavarro agreeing with @tbruyelle 's comment, let's move the test to tests/files/

Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

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

So in the GH review today, I noticed something odd: the code wasn't actually duplicated to the float32 counterpart. In fact, a different test (float7) was still panicking.

Funnily enough, it wouldn't work with SmallestNonZeroFloat64 (as the code didn't have the same logic to check whether the number was actually zero, as it was for float64). Anyway, I fixed the issues.

Now only waiting on @piux2 's review.

Copy link
Contributor

@piux2 piux2 left a comment

Choose a reason for hiding this comment

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

The code appears to be well-implemented. However, it seems that the usage of panic in the original code is intentional. It serves as a mechanism to handle the conversion of untyped BigDec constants. The function db.IsZero() seems to be utilized to distinguish a BigDec with a value of zero. This might act as a placeholder, awaiting the completion of the BigDec implementation to handle actual db values. I'll leave this for @jaekwon to further review and comment.

db:

*github.com/cockroachdb/apd.Decimal {
Form: Finite (0),
Negative: false,
Exponent: -1075,
Coeff: math/big.Int {
neg: false,
abs: math/big.nat len: 40, cap: 44, [2178133438652175037,13358389938759785811,2327513931309058258,13281776902381683126,12428501436096707604,11167376665378208913,7533419439590254135,11111329854973851249,381100326086554668,743925905065200102,16358649908316238435,7965390482101853370,8683260283480532003,10397040223970786542,13141442883861695791,13481094988647533917,13769060133574195735,4357625168117272887,2481288102022783458,15672420522825115297,242328635129153437,8057563909054409931,17290527069613993211,8205538365884795466,5757056363039532081,13174075699421158699,5499449782126738648,17921724191600867845,4561879847826661174,12842551368738208122,8993367378774161191,14552919816609462385,15503360519369613010,4353283772766715668,1833667960781563132,4601466743984949394,1077832616916714926,4715285661293679468,953411222142837528,1],},}

Here is the reference:
#306 (comment)

@jaekwon
Copy link
Contributor

jaekwon commented Oct 21, 2023

reading. also for reference here are the relevant spec details from Go:


From https://go.dev/ref/spec#Constant_expressions

Implementation restriction: A compiler may use rounding while computing untyped floating-point or complex constant expressions; see the implementation restriction in the section on constants. This rounding may cause a floating-point constant expression to be invalid in an integer context, even if it would be integral when calculated using infinite precision, and vice versa.

From https://go.dev/ref/spec#Constants

Implementation restriction: Although numeric constants have arbitrary precision in the language, a compiler may implement them using an internal representation with limited precision. That said, every implementation must:

Represent integer constants with at least 256 bits.
Represent floating-point constants, including the parts of a complex constant, with a mantissa of at least 256 bits and a signed binary exponent of at least 16 bits.
Give an error if unable to represent an integer constant precisely.
**Give an error if unable to represent a floating-point or complex constant due to overflow.**
Round to the nearest representable constant if unable to represent a floating-point or complex constant due to limits on precision.

These requirements apply both to literal constants and to the result of evaluating constant expressions.


The Go spec doesn't say anything about errors due to underflow which is what this issue is about. I wonder why...

@jaekwon
Copy link
Contributor

jaekwon commented Oct 21, 2023

Either way, ConvertUntypedBigdecTo is not the place to error on underflow, because erroring on underflow should only happen upon division, and this isn't the right place, so let's merge.

@thehowl thehowl merged commit 5cf3c71 into gnolang:master Oct 21, 2023
182 checks passed
thehowl added a commit to thehowl/gno that referenced this pull request Oct 21, 2023
gnolang#1185)

Added a unit test and an integration test trying to reproduce gnolang#1150 

Avoid unhandled errors.

- [x] Added new tests, or not needed, or not feasible
- [x] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [x] Updated the official documentation or not needed
- [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [x] Added references to related issues and PRs
- [x] Provided any useful hints for running manual tests
- [x] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).

---------

Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
Co-authored-by: Morgan Bazalgette <morgan@morganbaz.com>
gfanton pushed a commit to gfanton/gno that referenced this pull request Nov 9, 2023
gnolang#1185)

Added a unit test and an integration test trying to reproduce gnolang#1150 

Avoid unhandled errors.

- [x] Added new tests, or not needed, or not feasible
- [x] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [x] Updated the official documentation or not needed
- [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [x] Added references to related issues and PRs
- [x] Provided any useful hints for running manual tests
- [x] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).

---------

Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
Co-authored-by: Morgan Bazalgette <morgan@morganbaz.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: 🚀 Needed for Launch
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants