add tryinto instances for integer types to floats

[ghost]

this PR is about this issue.

[eliaslfox]

I don't think this is the right approach. All of these types have known bounds, the conversion code should just check if the input value is exactly representable in the output type. Also all of these conversions are infallible, so they should use Into not TryInto. Looking at the instances for Double-Float it looks like these already exist.

The ones that are missing are TryInto from I64, U64, IFix, and UFix. It's worth noting that we don't want TryInto conversions to lose precision, so if a value is within a floats range, but outside the safe range the conversion should fail.

[ghost]

I don't think this is the right approach. All of these types have known bounds, the conversion code should just check if the input value is exactly representable in the output type. Also all of these conversions are infallible, so they should use Into not TryInto. Looking at the instances for Double-Float it looks like these already exist.

The ones that are missing are TryInto from I64, U64, IFix, and UFix. It's worth noting that we don't want TryInto conversions to lose precision, so if a value is within a floats range, but outside the safe range the conversion should fail.

@eliaslfox should I start working on this again?

[eliaslfox]

If you want to. The issue is still open.

Does what I said about how to proceed make sense?

[ghost]

If you want to. The issue is still open.

Does what I said about how to proceed make sense?

so I should do TryInto from I64, U64, IFix, and UFix?

but I dont get this quote “so if a value is within a floats range, but outside the safe range the conversion should fail”.

[eliaslfox]

Floats have a range of values they can represent, but only some of that range represents numbers exactly. Larger values (or larger negative values) are imprecise. TryInto shouldn't lose precision, so when converting a value a float, if the values is outside the float's "safe range" then the conversion should fail.

[gefjon]

I could be misremembering, but I think the safe range for f64 (our Double-Float) is (-2^53 ..= 2^53), and for f32 (our Single-Float) (-2^24 ..= 2^24).

[ghost]

Floats have a range of values they can represent, but only some of that range represents numbers exactly. Larger values (or larger negative values) are imprecise. TryInto shouldn't lose precision, so when converting a value a float, if the values is outside the float's "safe range" then the conversion should fail.

Thanks. Can you provide a link to an example where there’s a test to check whether the values are outside the safe range? Thanks

here’s there’s a check whether it’s out of range https://github.com/coalton-lang/coalton/blob/main/library/math/conversions.lisp#L182. Is it like that?

here’s a stackoverflow answer
“ The idea is pretty simple: We first cast to float and then back to double and check whether the result is still the same. If d does not fit into float, then some precision would have been lost at the (float)d cast and thus the result would be different.”
https://stackoverflow.com/questions/26099818/how-quickly-check-whether-double-fits-in-float-java

[eliaslfox]

I think @gefjon is right about the ranges.

Just check that the value is within (-2^53, 2^53) before coercing to a double float.

The stack overflow answer will give confusing results because some values will coerce to floats, but that value plus or minus one won't. This is because some integers above 2^53 can be represented exactly as a double float, but not all of them.

See floating point for more information.

Also the Coalton code you linked should probably be updated to check integer ranges directly. I'm not sure if erroring when a value is out of range in coerce is guaranteed by the standard.

[ghost]

I think @gefjon is right about the ranges.

Just check that the value is within (-2^53, 2^53) before coercing to a double float.

The stack overflow answer will give confusing results because some values will coerce to floats, but that value plus or minus one won't. This is because some integers above 2^53 can be represented exactly as a double float, but not all of them.

See floating point for more information.

Also the Coalton code you linked should probably be updated to check integer ranges directly. I'm not sure if erroring when a value is out of range in coerce is guaranteed by the standard.

@eliaslfox want to take a look at my changes? let me know if I have to make any changes please. thanks

[eliaslfox]

A couple minor things. Also single-floats should have TryInto U32|I32 Single-Float.

[eliaslfox]

Write this as (Err ,(cl:concatenate ...)) so that it runs during macro expansion.

[eliaslfox]

Don't ignore errors here. The conversion should never fail, and if it does we want to know.

[eliaslfox]

Have the macro generate a define-instance without a coalton-toplevel. Then call the macro inside a toplevel like:

(coalton-toplevel
  (integer-tryinto-float ...)
  ...)

This allows the Coalton compiler to gives better error messages.

[eliaslfox]

You don't need to pass the type names as strings. Symbols can be converted to strings with cl:symbol-name.

[ghost]

A couple minor things. Also single-floats should have TryInto U32|I32 Single-Float.

@eliaslfox Ok I have addressed your review

[eliaslfox]

A couple minor things

[eliaslfox]

These can all be one coalton-toplevel

[eliaslfox]

Instead of (cl:quote ,lisp-float) write ',lisp-float

[eliaslfox]

Because this coercion should never fail, signal an error with cl:error instead returning Err.

[eliaslfox]

Also you need to wrap the macro definition in an eval-when like this because otherwise ccl and allegro get confused.

[eliaslfox]

Looks good

[eliaslfox]

It looks like there's merge conflicts. Resolve them locally with git rebase main, then I can merge this.

[ghost]

Looks good

@eliaslfox do i update the branch or do you take over from here?

[eliaslfox]

Don't update with the web ui, it'll create a merge commit. It needs to be done locally with git rebase to preserve a linear commit history.

[ghost]

It looks like there's merge conflicts. Resolve them locally with git rebase main, then I can merge this.

do i do this from my current branch patch-7 or from main?

[eliaslfox]

from patch-7

[eliaslfox]

Also you should squash your commits to a single commit.

[ghost]

@eliaslfox ok i have successfully rebased -- ie resolved all the conflicts manually. i was having trouble but after eating some chips i was able to to do it

[ghost]

@eliaslfox hey so i dont think theres merge conflicts now. the other day robert said he could squash the commits on his end. could you squash the commits? im fighting git right now. it seems like an endless cycle

[eliaslfox]

Yeah I can squash it tomorrow.

[ghost]

Yeah I can squash it tomorrow.

thanks. there shouldnt be merge conflicts. i resolved all the conflicts manually

[ghost]

@eliaslfox nice it got merged! so cool!