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

Add to Float arithmetic operators, is_finite,..., round, min/max. #1794

Merged
merged 20 commits into from Oct 23, 2018

Conversation

@Chris00
Copy link
Member

commented May 23, 2018

Remark: The round function calls to C because the latter is usually implemented as ASM. Note that naive round functions do not work.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented May 23, 2018

Since this PR adds a wealth of predicates could you maybe also add the is_integer predicate ? It's useful in various conversion scenarios.

@nojb

This comment has been minimized.

Copy link
Contributor

commented May 23, 2018

The AppVeyor failure is due to old versions of msvc not having round in their standard library. You will need to provide an implementation of round for them. @dra27 will probably know the full story about this.

Also, I think tests will be needed for the new functions.

@Chris00 Chris00 force-pushed the Chris00:float branch from ee65b0f to 07d1994 May 23, 2018

@Chris00

This comment has been minimized.

Copy link
Member Author

commented May 23, 2018

old versions of msvc not having round in their standard library. You will need to provide an implementation of round for them

I saw... ☹ I thought that was something of the past...

I'll try to do the additional work ASAP.

@alainfrisch alainfrisch added the stdlib label May 23, 2018

@Chris00 Chris00 force-pushed the Chris00:float branch 5 times, most recently from a32d1d2 to 7b09918 May 24, 2018

@Chris00

This comment has been minimized.

Copy link
Member Author

commented May 24, 2018

@dbuenzli Wish granted! ☺

Show resolved Hide resolved stdlib/float.ml

@Chris00 Chris00 force-pushed the Chris00:float branch from 7b09918 to 471bf73 May 29, 2018

@Chris00

This comment has been minimized.

Copy link
Member Author

commented Jun 1, 2018

@mshinwell It seems Flambda transforms -0. in +0. through an incorrect optimization. Here is an example that does not use the new primitives (it is important that the condition of the first if be “opaque”):

let z =
  let x = -0. and y = +0. in
  if mod_float x 1. >= 0. then
    x
  else if false then x else y

let () =
  printf "%g\n" (1. /. z)

prints inf (it should be -inf).

@xclerc

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2018

Here is a minimal reproduction case:

let f b =
  let x = -0. and y = +0. in
  if b then x else y

The problem is that flambda, when meeting the values of the
branches, uses = over float values. Thus considering that both
branches return the same value.

I will open a PR fixing the issue shortly.

@xclerc

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2018

#1810 fixes the issue mentioned in the previous comments.

@Chris00 Chris00 force-pushed the Chris00:float branch from 4e10c12 to 95ebccb Jun 2, 2018

@Chris00

This comment has been minimized.

Copy link
Member Author

commented Jun 2, 2018

Rebased on trunk.

when [x] or [y] is [nan]. Moreover [max (-0.) (+0.) = +0.] *)

val minmax : float -> float -> float * float
(** [minmax x y] returns [(x, y)] is [x <= y] and [(y, x)] otherwise.

This comment has been minimized.

Copy link
@xclerc

xclerc Jun 4, 2018

Contributor

"(...) returns (...) is (...)" should be "(...) returns (...) if (...)".

= "caml_nextafter_float" "caml_nextafter" [@@unboxed] [@@noalloc]
(** [nextafter x y] returns the next representable floating-point
value following [x] in the direction of [y]. If [y] is less than
[x], these functions will return the largest representable number

This comment has been minimized.

Copy link
@xclerc

xclerc Jun 4, 2018

Contributor

"these functions" seems to be from the C documentation...

This comment has been minimized.

Copy link
@Chris00

Chris00 Jun 4, 2018

Author Member

It is – the documentation is heavily inspired from the C one.

This comment has been minimized.

Copy link
@xclerc

xclerc Jun 4, 2018

Contributor

I meant: shouldn't it be "this function"?

This comment has been minimized.

Copy link
@Chris00

Chris00 Jun 4, 2018

Author Member

I modified it in the commit below.

@@ -204,6 +231,18 @@ external tanh : float -> float = "caml_tanh_float" "tanh"
[@@unboxed] [@@noalloc]
(** Hyperbolic tangent. Argument is in radians. *)

external trunc : float -> float = "caml_trunc_float" "caml_trunc"
[@@unboxed] [@@noalloc]
(** [trunc x] round [x] to the nearest integer whose absolute value is

This comment has been minimized.

Copy link
@xclerc

xclerc Jun 4, 2018

Contributor

"round" should be "rounds" for consistency.

(** [is_nan x] returns [true] if and only if [x] represents a [nan]. *)

val is_integer : float -> bool
(** Says whether the floating point is an integer. *)

This comment has been minimized.

Copy link
@xclerc

xclerc Jun 4, 2018

Contributor

"Says" should be "Say" for consistency.

(** [is_infinite x] says whether [x] is [infinity] or [neg_infinity]. *)

val is_nan : float -> bool
(** [is_nan x] returns [true] if and only if [x] represents a [nan]. *)

This comment has been minimized.

Copy link
@xclerc

xclerc Jun 4, 2018

Contributor

("[nan]" instead of "a [nan]"?)

This comment has been minimized.

Copy link
@Chris00

Chris00 Jun 4, 2018

Author Member

I'm not sure what is the better (given that there are several NaNs, even though we do not distinguish them here).

This comment has been minimized.

Copy link
@xclerc

xclerc Jun 4, 2018

Contributor

I am not sure, but I think the documentation tends
to use "nan" rather than "a nan" in other places.

This comment has been minimized.

Copy link
@Chris00

Chris00 Jun 4, 2018

Author Member

I can change it if you wish. Or maybe reformulate it as [x] is [nan].

This comment has been minimized.

Copy link
@xclerc

xclerc Jun 4, 2018

Contributor

I do not have a strong opinion on this question;
maybe @Octachron or @gasche will.

This comment has been minimized.

Copy link
@Octachron

Octachron Jun 4, 2018

Contributor

I like [x] is [nan]: it has the advantage of being correct when expanding it to [x] is not a number; moreover the Pervasives documentation already uses this wording.

This comment has been minimized.

Copy link
@Chris00

Chris00 Jun 21, 2018

Author Member

Done.

value following [x] in the direction of [y]. If [y] is less than
[x], these functions will return the largest representable number
less than [x]. If [x] equals [y], the function returns [y].
If [x] or [y] is a [nan], a [nan] is returned. *)

This comment has been minimized.

Copy link
@xclerc

xclerc Jun 4, 2018

Contributor

("[nan]" instead of "a [nan]"?)

@Chris00 Chris00 force-pushed the Chris00:float branch from d6480f4 to b87b241 Jun 21, 2018

@Chris00

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2018

Rebased.

@Chris00

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

suggest adding Float.{succ,pred,one,zero,minus_one}

Done.

@gasche

This comment has been minimized.

Copy link
Member

commented Oct 19, 2018

I have the impression that this is converging very nicely. @Chris00, @alainfrisch, are there things remaining to be done/discussed/decided to consider integrating this?

@dbuenzli
Copy link
Contributor

left a comment

Thanks @Chris00 !

@Chris00 Chris00 force-pushed the Chris00:float branch from 86c5d58 to 2fda33f Oct 19, 2018

@Chris00

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

@gasche Fine with me.

@xclerc

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2018

I have to say I have mixed feelings about pred/succ.

The introduction of one, zero, and minus_one to
be able to abstract over integer or float modules is fine,
but then the semantics of pred/succ is so different
that it is likely to cause more or less subtle problems.

@xclerc

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2018

To be clear, I would prefer to have the functions defined as:

let pred x = x -. 1.0
let succ x = x +. 1.0
@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2018

but then the semantics of pred/succ is so different

It's not different it gives you the next value greater than the current one in the domain of the data type.

@xclerc

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2018

I do understand your point of view, but I suspect some
code (arguably originally written with integers in mind)
would assume that succ x is x + 1.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2018

And maybe some code would simply like to get a value that is greater than the one that is given as argument. As far as I'm concerned pred and succ could also be defined on say type t = A | B | C.

@xclerc

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2018

I would also be fine with pred/succ on type t = A | B | C
because this type maps to a subset of integers...

My point is that most of the existing code abstracting over
IntK.{pred,succ} expects the natural properties between
these functions and addition to hold. Passing Float will then
lead to unexpected results.

I just wanted to share a concern, you and the author of the
PR will decide which definitions should be used.

@Chris00

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

My point is that most of the existing code abstracting over IntK.{pred,succ}

Are there examples?

In the end, the question is what semantics we want to have for pred and succ. As aliases for x -> X.add x X.one and x -> X.sub x X.one, I am not sure they are interesting — and to me, the successor is the one that comes after, which depends on the type and does not necessarily correspond to “+1”.

@xclerc

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2018

Are there examples?

To be fair, most of the examples I have in mind are so
closely related to integers that you would probably not
consider them. In Core, most occurrences of succ are
related to counters so one could reasonably move from
Int to Int{32,64} but not to Float.

In the end, the question is what semantics we want to have for pred and succ. As aliases for x -> X.add x X.one and x -> X.sub x X.one, I am not sure they are interesting — and to me, the successor is the one that comes after, which depends on the type and does not necessarily correspond to “+1”.

I won't say they are interesting as aliases, merely that they
are less surprising: replace Int32 with Float in a functor
application and you will approximately get the same results.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2018

I really prefer if we keep succ and pred as a convention for an ordered walk over the values of a type. As @Chris00 mentions I see little point of having that as an alias for add x one.

@xclerc

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2018

Out of curiosity, is nan the predecessor/successor
of a non-nan value?

@Chris00

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

@Chris00

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

succ x is nanx is nan

@Chris00 Chris00 force-pushed the Chris00:float branch from e5038b8 to 7e3a04e Oct 19, 2018

@xclerc

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2018

@Chris00 thanks!

@alainfrisch alainfrisch merged commit 00f9739 into ocaml:trunk Oct 23, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

Thanks @Chris00 for your efforts!

@Chris00 Chris00 deleted the Chris00:float branch Oct 23, 2018

damiendoligez added a commit to damiendoligez/ocaml that referenced this pull request Nov 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.