Reproduce Float-passing problem #9201

GregoryTravis · 2024-02-27T17:19:47Z

This applies @JaroslavTulach's fix in the comment below.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

GregoryTravis · 2024-02-27T17:23:28Z

test/Base_Tests/src/Data/Decimal_Spec.enso

+
+            vs.map v->
+                r = float_id v
+                IO.println "float_id "+v.to_text+" "+r.to_text


@JaroslavTulach I am seeing some unusual behavior here. I wonder if you get the same results when running Decimal_Spec.enso locally?

This test simply passes a Float to a Java function that returns its argument. It seems like the argument is changed when it reaches the Java in some cases.

float_id 9.223372036854776E19 9.223372036854776E19 float_id 9.223372036854778E18 9.223372036854778E18 float_id 9.223372036854776E18 9.223372036854776E18 float_id 9.223372036854776E18 9.223372036854776E18 float_id 9.223372036854776E18 9.223372036854776E18 float_id 9.223372036854775E18 NaN # unexpected float_id 9.223372036854774E18 NaN # unexpected float_id 9.2233720368547E18 NaN # unexpected float_id 9.223372036854E18 NaN # unexpected float_id 9.223372E18 NaN # unexpected float_id 9.2233720368547E17 5.148800401705924E-247 # unexpected float_id 9.2233720368547E16 1.7265376942307606E-302 # unexpected float_id 9.2233720368547E15 4.663754807431019E-308 # unexpected float_id 9.2233720368547E14 9.2233720368547E14 float_id 9.2233720368547E13 9.2233720368547E13 float_id 9.223372036854E12 9.223372036854E12 float_id 9.22337203685E11 9.22337203685E11 float_id 9.2233720368E10 9.2233720368E10 float_id 9.223372036E9 9.223372036E9 float_id 9.22337203E8 9.22337203E8 float_id 9.223372E7 9.223372E7 float_id 9223372.0 9223372.0 float_id 922337.0 922337.0 float_id 92233.0 92233.0 float_id 9223.0 9223.0 float_id 922.0 922.0 float_id -9.223372036854776E19 -9.223372036854776E19 float_id -9.223372036854778E18 -9.223372036854778E18 float_id -9.223372036854776E18 -0.0 # unexpected float_id -9.223372036854776E18 -0.0 # unexpected float_id -9.223372036854776E18 -0.0 # unexpected float_id -9.223372036854775E18 -5.06E-321 # unexpected float_id -9.223372036854774E18 -1.012E-320 # unexpected float_id -9.2233720368547E18 -3.74383E-319 # unexpected float_id -9.223372036854E18 -3.8349E-318 # unexpected float_id -9.223372E18 -1.8208678612E-313 # unexpected float_id -9.2233720368547E17 -8.390303882377555E246 # unexpected float_id -9.2233720368547E16 -2.6059089326772877E302 # unexpected float_id -9.2233720368547E15 -8.772742498128177E307 # unexpected float_id -9.2233720368547E14 -9.2233720368547E14 float_id -9.2233720368547E13 -9.2233720368547E13 float_id -9.223372036854E12 -9.223372036854E12 float_id -9.22337203685E11 -9.22337203685E11 float_id -9.2233720368E10 -9.2233720368E10 float_id -9.223372036E9 -9.223372036E9 float_id -9.22337203E8 -9.22337203E8 float_id -9.223372E7 -9.223372E7 float_id -9223372.0 -9223372.0 float_id -922337.0 -922337.0 float_id -92233.0 -92233.0 float_id -9223.0 -9223.0 float_id -922.0 -922.0

I wonder if you get the same results when running Decimal_Spec.enso locally?

Yes, I do. I am seeing the same results.

JaroslavTulach · 2024-02-28T15:05:05Z

This change seems to fix the problem:

enso$ git diff engine/polyglot-api/src/main/java/org/enso/polyglot/HostAccessFactory.java
diff --git engine/polyglot-api/src/main/java/org/enso/polyglot/HostAccessFactory.java engine/polyglot-api/src/main/java/org/enso/polyglot/HostAccessFactory.java
index 1f9cbbcf2c..150c43fc2f 100644
--- engine/polyglot-api/src/main/java/org/enso/polyglot/HostAccessFactory.java
+++ engine/polyglot-api/src/main/java/org/enso/polyglot/HostAccessFactory.java
@@ -21,11 +21,6 @@ public class HostAccessFactory {
         .allowIteratorAccess(true)
         .allowMapAccess(true)
         .allowAccessInheritance(true)
-        .targetTypeMapping(
-            Long.class,
-            Double.class,
-            (v) -> v != null && !longInSafeDoubleRange(v, LONG_MAX_SAFE_DOUBLE),
-            (v) -> Double.longBitsToDouble(v))
         .build();
   }

This conversion has been added in

Custom type conversion to double from large long #4099

by @hubertp. Hubert, looks like @GregoryTravis found edge cases when the conversion isn't working properly.

JaroslavTulach · 2024-03-01T04:26:49Z

We seem to have a failure related to Numeric values: can be passed in host calls without lossy coercion exception as that now ends:

Reason: An unexpected panic was thrown: (Unsupported_Argument_Types.Error [6907338656278321365] 
Cannot convert '6907338656278321365'(language: Java, type: java.lang.Long) to Java type 'double': 
Invalid or lossy primitive coercion.)
 at <enso> <anonymous><arg-1>(test/Base_Tests/src/Semantic/Java_Interop_Spec.enso:53:13-34)
 at <enso> null(Internal)
 at <enso> <anonymous>(Standard/Test/2024.1.1-dev/src/Group.enso:25:61-64)

I assume this is the test added by #4099 - its implementation was achieved by those five deleted lines. Now we have two options:

remove the test
don't delete the lines, but replace them with better ones

Removing the test can probably be done by anyone (with the blessing by someone from libraries team). Finding better lines is trickier. @hubertp please take a look and propose an alternative that wouldn't have drawbacks demonstrated in this PR and still satisfy the test.

JaroslavTulach · 2024-03-01T04:32:03Z

remove the test

Removing a test is a bad practice in general, but it may be the right solution in this case. I believe the test has been written before support for

EnsoBigInteger should be a BigInteger polyglot type #7213

has been put in place. Before the BigInteger interop got fully supported there was no way to get java.math.BigInteger into libraries Java code - as such the conversion to double might have been necessary. However now it is possible to accept java.math.BigInteger along double or long and handle the case differently.

If that's the case the test may be deleted or modified to benefit from the new big integer capabilities.

radeusgd · 2024-03-01T11:47:17Z

remove the test

Removing a test is a bad practice in general, but it may be the right solution in this case. I believe the test has been written before support for

EnsoBigInteger should be a BigInteger polyglot type #7213

has been put in place. Before the BigInteger interop got fully supported there was no way to get java.math.BigInteger into libraries Java code - as such the conversion to double might have been necessary. However now it is possible to accept java.math.BigInteger along double or long and handle the case differently.

If that's the case the test may be deleted or modified to benefit from the new big integer capabilities.

I agree that once we have proper BigInteger support, we can modify our methods to accept a BigInteger properly.

But I don't think that was the only motivation behind #4099.

One thing is APIs that we control (e.g. Moments in the test there), where we could add the BigInteger overload (and we probably also need a long overload there as well, right?)
Another thing is external APIs that we do not control, e.g. someone calling a Java function from some library that is only accepting double.

The problem is - in Enso we usually accept 2 wherever a Float would be accepted, and so the major question is - are we okay with not allowing that in polyglot calls? Shall we then add some kind of to_float conversion, that allows to convert a 2 into 2.0?

GregoryTravis · 2024-03-04T15:45:07Z

@hubertp What are your thoughts?

hubertp · 2024-03-04T21:13:26Z

@hubertp What are your thoughts?

Sorry, I replied to @JaroslavTulach in DM. I'm fine with ditching the test. The patch has been added prior to BigInteger support. I would also be fine in this case to make conversions explicit, rather than implicit, now.
The additional advantage of removing target mapping is that context initialization will be faster (that fragment actually took a non-negligible amount of time and could not be bypassed with other means).

GregoryTravis · 2024-03-04T21:39:07Z

@hubertp What are your thoughts?

Sorry, I replied to @JaroslavTulach in DM. I'm fine with ditching the test. The patch has been added prior to BigInteger support. I would also be fine in this case to make conversions explicit, rather than implicit, now. The additional advantage of removing target mapping is that context initialization will be faster (that fragment actually took a non-negligible amount of time and could not be bypassed with other means).

Should this PR be merged on its own, or does it need other changes with it?

JaroslavTulach · 2024-03-06T08:56:30Z

Should this PR be merged on its own, or does it need other changes with it?

Once the CI is green and code owners are fine with the PR, it can be merged.

radeusgd · 2024-03-06T11:08:54Z

std-bits/base/src/main/java/org/enso/base/numeric/Number_Utils.java

+package org.enso.base.numeric;
+
+public class Number_Utils {
+  // For testing only.
+  public static Double floatId(Double d) {
+    return d;
+  }
+}


If this is only used for tests, it should be moved to test/Base_Tests/polyglot-sources. This is a Java lib specifically compiled for Base_Tests.

radeusgd

Looks good, but I'd like to move the helper to the dedicated test library instead.

btw. @JaroslavTulach curious note for you - as you can see, we have a separate JAR built for Base_Tests that includes some utils which are used for the tests but are not meant to be 'shipped' for users of Base library. Do you think we can retain this structure if we merge the tests to be parts of their libraries? (I guess it would not be a big issue if we cannot, just wondering)

GregoryTravis added 5 commits February 27, 2024 11:47

wip

1f59ba5

and negative

f004076

wip

c5084cf

wip

b14aa95

wip

17708d1

GregoryTravis commented Feb 27, 2024

View reviewed changes

enso-bot bot mentioned this pull request Feb 29, 2024

Autoscoping of constructors #8645

Closed

GregoryTravis added 2 commits February 29, 2024 11:44

Merge branch 'develop' into 8982-reproduce-id-problem

28fd13b

apply Jaroslav fix

e04fcf1

GregoryTravis mentioned this pull request Feb 29, 2024

Fix edge case in passing Floats to Java #9228

Closed

5 tasks

GregoryTravis marked this pull request as ready for review February 29, 2024 16:58

GregoryTravis requested review from jdunkerley, radeusgd, AdRiley, 4e6, hubertp and Akirathan as code owners February 29, 2024 16:58

GregoryTravis requested a review from JaroslavTulach February 29, 2024 16:58

GregoryTravis added the CI: No changelog needed Do not require a changelog entry for this PR. label Feb 29, 2024

GregoryTravis added 2 commits February 29, 2024 15:30

unused

9a3215c

unused

843ff82

JaroslavTulach approved these changes Mar 6, 2024

View reviewed changes

radeusgd reviewed Mar 6, 2024

View reviewed changes

radeusgd approved these changes Mar 6, 2024

View reviewed changes

GregoryTravis added 2 commits March 6, 2024 10:40

Merge branch 'develop' into 8982-reproduce-id-problem

457c35a

move Number_Utils to polyglot-sources

df00be1

GregoryTravis mentioned this pull request Mar 6, 2024

Decimal type: constuctors, comparisons, and arithmetic #9272

Merged

5 tasks

Merge branch 'develop' into 8982-reproduce-id-problem

a37f9a1

hubertp approved these changes Mar 7, 2024

View reviewed changes

GregoryTravis added 4 commits March 7, 2024 09:17

Merge branch 'develop' into 8982-reproduce-id-problem

f49e1f2

Merge branch 'develop' into 8982-reproduce-id-problem

e984bff

remove test

b461f88

Merge branch 'develop' into 8982-reproduce-id-problem

3fe9e2d

GregoryTravis added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Mar 13, 2024

Merge branch 'develop' into 8982-reproduce-id-problem

f4645a1

GregoryTravis added the CI: Ready to merge This PR is eligible for automatic merge label Mar 14, 2024

mergify bot merged commit dc84317 into develop Mar 14, 2024
39 of 41 checks passed

mergify bot deleted the 8982-reproduce-id-problem branch March 14, 2024 19:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reproduce Float-passing problem #9201

Reproduce Float-passing problem #9201

GregoryTravis commented Feb 27, 2024 •

edited

Loading

GregoryTravis Feb 27, 2024 •

edited

Loading

JaroslavTulach Feb 28, 2024

JaroslavTulach commented Feb 28, 2024 •

edited

Loading

JaroslavTulach commented Mar 1, 2024

JaroslavTulach commented Mar 1, 2024

radeusgd commented Mar 1, 2024

GregoryTravis commented Mar 4, 2024

hubertp commented Mar 4, 2024

GregoryTravis commented Mar 4, 2024

JaroslavTulach commented Mar 6, 2024

radeusgd Mar 6, 2024

GregoryTravis Mar 6, 2024

radeusgd left a comment

Reproduce Float-passing problem #9201

Reproduce Float-passing problem #9201

Conversation

GregoryTravis commented Feb 27, 2024 • edited Loading

Checklist

GregoryTravis Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

JaroslavTulach Feb 28, 2024

Choose a reason for hiding this comment

JaroslavTulach commented Feb 28, 2024 • edited Loading

JaroslavTulach commented Mar 1, 2024

JaroslavTulach commented Mar 1, 2024

radeusgd commented Mar 1, 2024

GregoryTravis commented Mar 4, 2024

hubertp commented Mar 4, 2024

GregoryTravis commented Mar 4, 2024

JaroslavTulach commented Mar 6, 2024

radeusgd Mar 6, 2024

Choose a reason for hiding this comment

GregoryTravis Mar 6, 2024

Choose a reason for hiding this comment

radeusgd left a comment

Choose a reason for hiding this comment

GregoryTravis commented Feb 27, 2024 •

edited

Loading

GregoryTravis Feb 27, 2024 •

edited

Loading

JaroslavTulach commented Feb 28, 2024 •

edited

Loading