Feat/float as decimal #1
Conversation
WalkthroughThe changes mainly involve updating the implementation and handling of float values across different parts of the codebase. The old float operations using Changes
Sequence DiagramsSince the changes primarily revolve around the internal handling of float values and updating test cases, creating a meaningful sequence diagram would not provide additional clarity. The modifications do not significantly alter the high-level control flow or introduce new features warranting sequence diagrams. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
amimart
force-pushed
the
feat/float-as-decimal
branch
from
05361f9 to
4f4f1ff
Compare
trigonometric functions have been removed
amimart
force-pushed
the
feat/float-as-decimal
branch
from
4f4f1ff to
f4bc73c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
engine/float.go (1)
Line range hint
76-90:
Formatting logic inWriteTermis correct but could be optimized.The function correctly formats the
Floatvalue, but the multipleWritecalls could be combined to improve performance.- _, _ = ew.Write([]byte(" ")) - _, _ = ew.Write([]byte("(")) - _, _ = ew.Write([]byte(s)) - _, _ = ew.Write([]byte(")")) + ew.Write([]byte(fmt.Sprintf("%s(%s)%s", " ", s, ")")))
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
Files selected for processing (17)
- engine/atom_test.go (1 hunks)
- engine/builtin_test.go (16 hunks)
- engine/compound_test.go (1 hunks)
- engine/env.go (1 hunks)
- engine/float.go (2 hunks)
- engine/float_test.go (3 hunks)
- engine/integer_test.go (1 hunks)
- engine/number.go (22 hunks)
- engine/number_test.go (10 hunks)
- engine/parser.go (2 hunks)
- engine/parser_test.go (6 hunks)
- engine/stream_test.go (1 hunks)
- engine/term_test.go (1 hunks)
- engine/variable_test.go (1 hunks)
- interpreter_test.go (6 hunks)
- solutions.go (3 hunks)
- solutions_test.go (5 hunks)
Files not summarized due to errors (1)
- engine/number_test.go: Error: Message exceeds token limit
Additional context used
golangci-lint
engine/number.go
12-12: var
oneFloatis unused(unused)
Additional comments not posted (75)
engine/term_test.go (1)
49-49: Approved: Correct integration ofNewFloatFromInt64.The change from
Float(0)toNewFloatFromInt64(0)aligns with the new float handling logic.engine/integer_test.go (1)
49-49: Approved: Correct integration ofNewFloatFromInt64.The change from
Float(1)toNewFloatFromInt64(1)aligns with the new float handling logic.engine/float_test.go (3)
10-10: Approved: Correct integration ofNewFloatFromInt64.The change ensures that
NewFloatFromInt64(0)correctly implements theNumberinterface.
20-24: Approved: Correct integration ofnewFloatFromFloat64Must.The changes ensure that float values are correctly created and formatted using
newFloatFromFloat64Must.
46-52: Approved: Correct integration ofNewFloatFromInt64.The changes ensure that float values are correctly compared using
NewFloatFromInt64.engine/atom_test.go (1)
54-54: Approved: Correct integration ofNewFloatFromInt64.The change from
Float(1.0)toNewFloatFromInt64(1)aligns with the new float handling logic.engine/variable_test.go (1)
50-50: Change looks good.The change from
Float(0)toNewFloatFromInt64(0)is consistent with the new implementation usingapd.Decimal.engine/float.go (16)
4-5: Import statements are appropriate.The addition of the
apdlibrary is necessary for the new decimal implementation.
11-16: Good documentation and struct definition.The comments clearly explain the rationale behind the new
Floattype, and the struct definition is straightforward.
18-24: Context initialization is appropriate.The
decimal128Ctxinitialization is consistent with the requirements for arbitrary precision decimals.
26-33: Constructor forFloatfrom string is well-implemented.The function correctly initializes a
Floatfrom a string and handles errors appropriately.
35-40: Constructor forFloatfrom int64 is well-implemented.The function correctly initializes a
Floatfrom an int64.
42-69: Error handling for decimal conditions is comprehensive.The function covers all possible error conditions and maps them to appropriate error values.
113-117: Comparison logic forFloatis correct.The function correctly compares
Floatvalues usingapd.Decimal's comparison method.
119-121: String representation forFloatis correct.The function correctly returns the string representation of the
Floatvalue.
123-125: Negative check forFloatis correct.The function correctly checks if the
Floatvalue is negative.
127-129: Positive check forFloatis correct.The function correctly checks if the
Floatvalue is positive.
131-133: Zero check forFloatis correct.The function correctly checks if the
Floatvalue is zero.
135-137: Equality check forFloatis correct.The function correctly checks if two
Floatvalues are equal.
139-141: Greater than check forFloatis correct.The function correctly checks if one
Floatvalue is greater than another.
143-145: Greater than or equal check forFloatis correct.The function correctly checks if one
Floatvalue is greater than or equal to another.
147-149: Less than check forFloatis correct.The function correctly checks if one
Floatvalue is less than another.
151-153: Less than or equal check forFloatis correct.The function correctly checks if one
Floatvalue is less than or equal to another.engine/env.go (1)
309-317: Unification logic forFloatandIntegeris correct.The function correctly handles unification of
FloatandIntegertypes, ensuring the new decimal implementation is used.solutions.go (1)
144-148: Conversion logic forengine.Floatto string is correct.The function correctly converts
engine.Floatto string, ensuring the new decimal implementation is used.engine/compound_test.go (1)
76-76: Ensure consistency in float handling.The change from
Float(1)toNewFloatFromInt64(1)is consistent with the new float handling logic. Ensure that all instances of float usage in the tests are updated similarly.solutions_test.go (4)
85-87: Ensure correct float handling and string conversion.The change from
Float(1)toNewFloatFromInt64(1)and the conversion of the result to a string is consistent with the new float handling logic. Ensure that similar changes are applied throughout the codebase.
228-233: Ensure correct float handling and string conversion in example.The change from
float64tostringfor the float field in the example is consistent with the new float handling logic. Ensure that similar changes are applied throughout the codebase.
247-254: Ensure correct float handling and string conversion in tagged example.The change from
float64tostringfor the float field in the tagged example is consistent with the new float handling logic. Ensure that similar changes are applied throughout the codebase.
270-277: Ensure correct float handling and string conversion in list example.The change from
float64tostringfor the float field in the list example is consistent with the new float handling logic. Ensure that similar changes are applied throughout the codebase.engine/parser_test.go (7)
12-22: Ensure equality assertion for new float handling.The
assertEqualFloatAwarefunction ensures that the new float handling logic is correctly compared in tests. This is a good addition to handle the newapd.Decimaltype.
84-87: Ensure correct float handling in parser term tests.The change from
Float(1.0)toNewFloatFromInt64(1)and similar changes are consistent with the new float handling logic. Ensure that similar changes are applied throughout the codebase.
182-191: Ensure correct float handling in parser term tests.The usage of
assertEqualFloatAwareensures correct comparison of floats in the parser term tests. This is consistent with the new float handling logic.
210-218: Ensure correct float handling in parser replace tests.The change from
float64toNewFloatFromInt64andnewFloatFromFloat64Mustis consistent with the new float handling logic. Ensure that similar changes are applied throughout the codebase.
224-225: Ensure correct float handling in parser replace tests.The change from
float64toNewFloatFromInt64andnewFloatFromFloat64Mustis consistent with the new float handling logic. Ensure that similar changes are applied throughout the codebase.
236-242: Ensure correct argument handling in parser replace tests.The test cases for handling too few and too many arguments ensure that the parser correctly handles placeholder replacements. This is consistent with the new float handling logic.
292-295: Ensure correct float handling in parser number tests.The change from
float64tonewFloatFromFloat64Mustis consistent with the new float handling logic. Ensure that similar changes are applied throughout the codebase.engine/stream_test.go (1)
82-82: Ensure consistency in float handling.The change from
Float(1)toNewFloatFromInt64(1)is consistent with the new float handling logic. Ensure that all instances of float usage in the tests are updated similarly.engine/parser.go (1)
806-808: LGTM! But verify the usage ofNewFloatFromString.The changes correctly handle the negative sign and create a new float from the string representation.
However, ensure that
NewFloatFromStringis used correctly throughout the codebase.Verification successful
Verified: Correct usage of
NewFloatFromStringThe function
NewFloatFromStringis used correctly in the following locations:
engine/parser.go: Ensures the string has the correct sign before conversion.engine/number.go: Initializes a constant value.engine/number_test.go: Part of a test utility function that panics if the conversion fails.All usages align with the function's purpose of converting a string to a
Floattype.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `NewFloatFromString` to ensure it is used correctly throughout the codebase. # Test: Search for the function usage. Expect: Correct usage of `NewFloatFromString`. rg --type go $'NewFloatFromString'Length of output: 318
Script:
#!/bin/bash # Extract the relevant code snippets to verify the correct usage of `NewFloatFromString`. # Extract 10 lines before and after the usage in parser.go rg --type go -A 10 -B 10 'NewFloatFromString' engine/parser.go # Extract 10 lines before and after the usage in number.go rg --type go -A 10 -B 10 'NewFloatFromString' engine/number.go # Extract 10 lines before and after the usage in number_test.go rg --type go -A 10 -B 10 'NewFloatFromString' engine/number_test.go # Extract the function definition from float.go rg --type go -A 20 -B 0 'func NewFloatFromString' engine/float.goLength of output: 2038
engine/number.go (6)
Line range hint
770-785:
LGTM!The changes correctly use the
NewFloatFromInt64function to handle mixed mode operations in themaxfunction.
Line range hint
808-823:
LGTM!The changes correctly use the
NewFloatFromInt64function to handle mixed mode operations in theminfunction.
608-619: LGTM! But verify the usage ofdecimal128Ctx.Exp.The changes correctly use the
apdlibrary for arbitrary-precision decimal arithmetic in theexpfunction.However, ensure that
decimal128Ctx.Expis used correctly throughout the codebase.
582-603: LGTM! But verify the usage ofdecimal128Ctx.Pow.The changes correctly use the
apdlibrary for arbitrary-precision decimal arithmetic in thepowerfunction.However, ensure that
decimal128Ctx.Powis used correctly throughout the codebase.
624-639: LGTM! But verify the usage ofdecimal128Ctx.Ln.The changes correctly use the
apdlibrary for arbitrary-precision decimal arithmetic in thelogfunction.However, ensure that
decimal128Ctx.Lnis used correctly throughout the codebase.Verification successful
Verify the definition and context of
decimal128Ctx.The usage of
decimal128Ctx.Lnis found only once in the codebase. Ensure thatdecimal128Ctxis correctly defined and used in theengine/number.gofile.
Verify the definition of
decimal128Ctxin the entire codebase.The
decimal128Ctxis used multiple times in theengine/number.gofile, but its definition is not shown. Ensure thatdecimal128Ctxis correctly defined and used throughout the codebase.
Verify the definition and configuration of
decimal128Ctxinengine/float.go.The
decimal128Ctxis defined as a variable of typeapd.Contextinengine/float.go. Ensure that it is correctly configured and used.
Verified: The usage of
decimal128Ctx.Lnis correct.The
decimal128Ctxis correctly defined and configured inengine/float.gowith appropriate precision and exponent range for arbitrary-precision decimal arithmetic. The usage ofdecimal128Ctx.Lninengine/number.gois correct.
decimal128Ctxconfiguration:
- Precision: 34
- MaxExponent: 6144
- MinExponent: -6143
- Traps: apd.DefaultTraps
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `decimal128Ctx.Ln` to ensure it is used correctly throughout the codebase. # Test: Search for the function usage. Expect: Correct usage of `decimal128Ctx.Ln`. rg --type go $'decimal128Ctx.Ln'Length of output: 89
Script:
#!/bin/bash # Description: Verify the definition and context of `decimal128Ctx` in the engine/number.go file. # Search for the definition of `decimal128Ctx` in the engine/number.go file. rg --type go 'decimal128Ctx' engine/number.goLength of output: 657
Script:
#!/bin/bash # Description: Verify the definition of `decimal128Ctx` in the entire codebase. # Search for the definition of `decimal128Ctx` in the entire codebase. rg --type go 'decimal128Ctx'Length of output: 1023
Script:
#!/bin/bash # Description: Review the definition and configuration of `decimal128Ctx` in `engine/float.go`. # Display the definition of `decimal128Ctx` in the `engine/float.go` file. rg --type go --context 10 'var decimal128Ctx = apd.Context' engine/float.goLength of output: 759
644-659: LGTM! But verify the usage ofdecimal128Ctx.Sqrt.The changes correctly use the
apdlibrary for arbitrary-precision decimal arithmetic in thesqrtfunction.However, ensure that
decimal128Ctx.Sqrtis used correctly throughout the codebase.Verification successful
Verify the usage of
decimal128Ctx.Sqrtinengine/number.go.The search results show that
decimal128Ctx.Sqrtis only used inengine/number.go. Ensure this function is used correctly by examining the context and related imports.
Verify the complete import section in
engine/number.go.The context of the
sqrtfunction appears correct, but it's essential to confirm thatdecimal128Ctxis properly imported and initialized.
Verify the initialization of
decimal128Ctxinengine/number.go.The
apdpackage is correctly imported. Next, ensure thatdecimal128Ctxis properly initialized.
Verify the initialization of
decimal128Ctxin the codebase.The search results show multiple usages of
decimal128Ctxfunctions but do not indicate its initialization. Ensure thatdecimal128Ctxis properly initialized by searching the entire codebase.
Verified: Correct usage of
decimal128Ctx.Sqrt.The
decimal128Ctxis properly initialized inengine/float.goand used correctly inengine/number.go.
engine/float.go: Initialization ofdecimal128Ctxas anapd.Contextvariable.engine/number.go: Correct usage ofdecimal128Ctx.Sqrt.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `decimal128Ctx.Sqrt` to ensure it is used correctly throughout the codebase. # Test: Search for the function usage. Expect: Correct usage of `decimal128Ctx.Sqrt`. rg --type go $'decimal128Ctx.Sqrt'Length of output: 93
Script:
#!/bin/bash # Description: Verify the context and correctness of `decimal128Ctx.Sqrt` usage in `engine/number.go`. # Display the surrounding lines of the usage to understand the context. rg --context 10 --type go 'decimal128Ctx.Sqrt' engine/number.go # Check the imports in the file to ensure `decimal128Ctx` is properly imported and initialized. rg --type go 'import' engine/number.goLength of output: 538
Script:
#!/bin/bash # Description: Gather the complete import section from `engine/number.go` to ensure `decimal128Ctx` is properly imported and initialized. # Display the complete import section. rg --type go --context 20 '^import' engine/number.goLength of output: 522
Script:
#!/bin/bash # Description: Verify the initialization of `decimal128Ctx` in `engine/number.go` to ensure it is correctly set up. # Search for the initialization of `decimal128Ctx`. rg --type go 'decimal128Ctx' engine/number.goLength of output: 657
Script:
#!/bin/bash # Description: Verify the initialization of `decimal128Ctx` in the codebase to ensure it is correctly set up. # Search for the initialization of `decimal128Ctx`. rg --type go 'decimal128Ctx'Length of output: 1023
engine/number_test.go (10)
14-22: LGTM!The
newFloatFromFloat64Mustfunction is correctly implemented, converting a float64 to aFloatusingapd.Decimalwith proper error handling.
24-31: LGTM!The
newFloatFromStringMustfunction is correctly implemented, converting a string to aFloatusingNewFloatFromStringwith proper error handling.
33-40: LGTM!The
mulMustfunction is correctly implemented, multiplying twoFloatvalues usingmulFwith proper error handling.
Line range hint
44-245:
LGTM!The test cases in
TestIshave been correctly updated to useNewFloatFromInt64andnewFloatFromFloat64Must, ensuring they align with the new float handling logic.
292-306: LGTM!The test cases in
TestEqualhave been correctly updated to useNewFloatFromInt64, ensuring they align with the new float handling logic.
339-341: LGTM!The test cases in
TestNotEqualhave been correctly updated to useNewFloatFromInt64, ensuring they align with the new float handling logic.
366-368: LGTM!The test cases in
TestLessThanhave been correctly updated to useNewFloatFromInt64, ensuring they align with the new float handling logic.
393-395: LGTM!The test cases in
TestGreaterThanhave been correctly updated to useNewFloatFromInt64, ensuring they align with the new float handling logic.
420-422: LGTM!The test cases in
TestLessThanOrEqualhave been correctly updated to useNewFloatFromInt64, ensuring they align with the new float handling logic.
447-449: LGTM!The test cases in
TestGreaterThanOrEqualhave been correctly updated to useNewFloatFromInt64, ensuring they align with the new float handling logic.interpreter_test.go (4)
727-727: Verify the correctness of replacing float with integer.The test case has been modified to use an integer instead of a float. Ensure that this change aligns with the intended behavior and does not affect the test's validity.
772-772: Verify the correctness of replacing float with string.The test case has been modified to use a string instead of a float. Ensure that this change aligns with the intended behavior and does not affect the test's validity.
791-791: Verify the correctness of replacing float with string.The test case has been modified to use a string instead of a float. Ensure that this change aligns with the intended behavior and does not affect the test's validity.
795-795: Verify the correctness of replacing float with string.The test case has been modified to use a string instead of a float. Ensure that this change aligns with the intended behavior and does not affect the test's validity.
engine/builtin_test.go (16)
402-402: LGTM!The change from
Float(1)toNewFloatFromInt64(1)aligns with the PR objective of using arbitrary precision decimals.
456-456: LGTM!The change from
Float(1)toNewFloatFromInt64(1)aligns with the PR objective of using arbitrary precision decimals.
518-518: LGTM!The change from
Float(1.0)tonewFloatFromFloat64Must(1.0)aligns with the PR objective of using arbitrary precision decimals.
635-636: LGTM!The changes from
Float(1.1)andFloat(1.5)tonewFloatFromFloat64Must(1.1)andnewFloatFromFloat64Must(1.5)align with the PR objective of using arbitrary precision decimals.Also applies to: 643-643
785-785: LGTM!The change from
Float(1.1)tonewFloatFromFloat64Must(1.1)aligns with the PR objective of using arbitrary precision decimals.
2107-2108: LGTM!The changes from
Float(3.0)tonewFloatFromFloat64Must(3.0)align with the PR objective of using arbitrary precision decimals.
5526-5526: LGTM!The change from
Float(1.23)tonewFloatFromFloat64Must(1.23)aligns with the PR objective of using arbitrary precision decimals.
5871-5871: LGTM!The change from
Float(23.4)tonewFloatFromFloat64Must(23.4)aligns with the PR objective of using arbitrary precision decimals.
5882-5882: LGTM!The change from
Float(23.4)tonewFloatFromFloat64Must(23.4)aligns with the PR objective of using arbitrary precision decimals.
5895-5895: LGTM!The change from
Float(23.4)tonewFloatFromFloat64Must(23.4)aligns with the PR objective of using arbitrary precision decimals.
5904-5904: LGTM!The changes from
Float(3.3)tonewFloatFromFloat64Must(3.3)align with the PR objective of using arbitrary precision decimals.Also applies to: 5910-5910
6020-6020: LGTM!The changes from
Float(33.0)tonewFloatFromFloat64Must(33.0)align with the PR objective of using arbitrary precision decimals.Also applies to: 6023-6023
6037-6037: LGTM!The changes from
Float(4.2)tonewFloatFromFloat64Must(4.2)align with the PR objective of using arbitrary precision decimals.Also applies to: 6040-6040
7165-7166: LGTM!The change from
Float(1)toNewFloatFromInt64(1)aligns with the PR objective of using arbitrary precision decimals.
7188-7189: LGTM!The change from
Float(1)toNewFloatFromInt64(1)aligns with the PR objective of using arbitrary precision decimals.
7209-7210: LGTM!The change from
Float(0)tonewFloatFromFloat64Must(0)aligns with the PR objective of using arbitrary precision decimals.
This PR brings changes in the behaviour of floats, by using under the hood arbitrary precision decimals following the GDA specification, using the https://github.com/cockroachdb/apd library.
Details
The change is motivated by two main reasons:
To avoid radical changes with the original prolog project I propose to change the behaviour of floating point by a decimal implementation instead of removing this type and introducing a new one.
The chosen implementation is a 128bits decimal with 34 digits of precision, mainly guided by the following issue: cosmos/cosmos-sdk#11783 on cosmos sdk.
Finally, for deterministic purpose, some integer arithmetic operations have also been changed using the same implementation.
Notable changes
Some changes are inherent to the use of decimal:
Some functors have been removed because too difficult to implement:
sincostanasinacosatanatan2Regarding solutions scanning, float types are no longer supported, the prolog float type is now marshalled as a
string.The possibility to provide float placeholders in prolog code has been removed.
Tests have been updated according to those changes.