BigDecimalBuilder and arithmetic operations.
#9950
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| _ -> x | ||
|
|
||
| java_to_enso x = case x of | ||
| _ : BigDecimal -> Decimal.Value x |
There was a problem hiding this comment.
java_to_enso is a conversion function that wraps java.math.BigDecimal into Enso's Decimal by calling the Value constructor. That's correct wrapping. What kind of problem is associated with it? Are you searching for alternatives?
You can call the factory in Java, if you want. If you create a method:
public static Object factory(Function<BigDecimal,Object> f) {
return f.apply(BigDecimal.valueOf(1));
}and call it from Enso as
IO.println <| factory Decimal.Valuethen you should get Decimal.Value 1. Other than this, I am not sure what to do/change.
radeusgd
reviewed
May 27, 2024
| @@ -651,6 +651,8 @@ add_specs suite_builder = | |||
|
|
|||
| Decimal.new "12" . div (Decimal.new "0") . should_fail_with Arithmetic_Error | |||
|
|
|||
| ((Decimal.new "1") / (Decimal.new "3")) . should_fail_with Arithmetic_Error | |||
There was a problem hiding this comment.
Why does it fail? I don't understand.
There was a problem hiding this comment.
This fails because Decimal defaults to infinite precision unless you use an explicit Math_Context value.
The solution to this might be to use a Math_Context by default, but doing so might be unexpected, and lose some of the value of Decimal, which is that it attempts to handle precision automatically. I think we'll have to use this in anger before we can make that decision, to decide what the proper defaults should be.
There was a problem hiding this comment.
I see. I don't think it's a problem for it to fail by default, I just asked because I did not understand.
What is the error message in that case? I think we should try to ensure the error message is clear and ideally proposes the fix (use Math_Context to limit precision). Do you think that would be possible?
radeusgd
approved these changes
May 27, 2024
|
The benchmark comparison below shows that for Before: After: |
|
I also followed @JaroslavTulach's suggestion to try doing the conversion in Java rather than Enso. This was consistently slower for Conversion in Enso: Conversion in Java: |
GregoryTravis
added
CI: Ready to merge
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request Description
Implements
BigDecimalBuilderand arithmetic operations.Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.