ocaml/ocaml

Merged
merged 10 commits into from Mar 16, 2018

Conversation

Projects
None yet
Contributor

nojb commented Feb 27, 2018 • edited

 Let the floodgates open! 😄 Some remarks: Most names are the same, except that for analogy with the other numeric modules I renamed: abs_float -> Float.abs, mod_float -> Float.rem, string_of_float -> Float.to_string, etc. Do we want to deprecate the "old" Pervasives float functions? See also #1010, #964, #944, #1294, #1354. Comments welcome!
Member

yallop commented Feb 27, 2018

 It would be good to have a Float.Array submodule like this: module Array : sig type t = floatarray external create : int -> t = "caml_floatarray_create" external length : t -> int = "%floatarray_length" external get : t -> int -> float = "%floatarray_safe_get" external set : t -> int -> float -> unit = "%floatarray_safe_set" external unsafe_get : t -> int -> float = "%floatarray_unsafe_get" external unsafe_set : t -> int -> float -> unit = "%floatarray_unsafe_set" end and to make Array.Floatarray an alias for this module. See previous discussion under the "Configure-time option for float array optimization" PR.

alainfrisch reviewed Feb 27, 2018

 (** [modf f] returns the pair of the fractional and integral part of [f]. *) type t = float

alainfrisch Feb 27, 2018

Contributor

I think it would be a nice convention if such module related to a specific type would come with the type t = .... alias as their first exposed component, perhaps immediately followed by common functions (compare, equal).

Let's also add Float.hash.

xclerc Feb 27, 2018

Contributor

Well, in modules such as Int32 or String they currently appear at the end.
Perhaps the convention you suggest should be the subject of another PR,
to be applied to all relevant modules.

nojb Feb 27, 2018

Author Contributor

I think this will require that I duplicate the definition of Hashtbl.hash to avoid a circular dependency (Array -> Float -> Hashtbl -> Array).

alainfrisch Feb 27, 2018

Contributor

Hashtbl.hash is a one-line wrapper around a primitive, so the duplication is fine (there are plenty of other cases of such duplication in the stdlib).

And IMO it would also be ok to drop the dependency from Array to Float (by duplicating the current definition of the module, marking Array.Float as deprecated, and only extending Float.Array going forward).

Contributor Author

nojb commented Feb 27, 2018

 It would be good to have a Float.Array submodule like this: This is now done.

alainfrisch reviewed Feb 27, 2018

 a negative integer if [x] is less than [y], and a positive integer if [x] is greater than [y]. The ordering implemented by [compare] is compatible with the comparison predicates [=], [<] and [>] defined above, with one difference on the treatment of the float value

alainfrisch Feb 27, 2018

Contributor

Should these predicates be exposed in Float (it seems they aren't, currently)? If so, perhaps in a sub-mode Floats.Ops?

Also, concerning the equality, I always found it unfortunate that there is no easy way to create a "Leibniz-equality" for floats, which is needed to implement memoization correctly (for instance). compare x y = 0 is also good, but it doesn't make the difference between positive and negative, which behave differently (for x -> 1/x).

Would it be a good time to add such a function (or just the associated total ordering)?

xclerc Feb 27, 2018

Contributor

Could we rely on a another compare function, that would compare
float values by first converting them into int64 ones (bit patterns)?

nojb Feb 27, 2018 • edited

Author Contributor

So far there seem to be 3 notions of comparison which are useful in different situations:

• Pervasives.compare: total order, useful for using with Map.Make, etc.
• Pervasives.(<): IEEE 754 comparison, can detect if x is nan with x <> x.
• "bitwise", "Leibniz" or "physical"-equality: useful for memoization, etc.

Why not expose all of them? We need to decide on suitable names. And we need to choose one of them for equal.

alainfrisch Feb 27, 2018

Contributor

Note: bitwise would differentiate nan with different encodings, while I'm not sure we can make the difference otherwise (well, except inspecting the bits).

bluddy Feb 27, 2018

I think we want a total order as our main comparison function, and this should be the only kind of comparison exposed cleanly.

For nan detection, an is_nan function makes more sense to me than needing any kind of comparison. Bitwise comparison is a niche and should have an appropriately specific name.

alainfrisch Feb 27, 2018

Contributor

I think we want a total order as our main comparison function, and this should be the only kind of comparison exposed cleanly.

The question is which one? The current one is fine for many uses, but it collapses -0. and 0. which makes it dangerous to use in some cases. But a refined version with -0. < 0. would not be suitable in all cases either. Both are total.

For nan detection, we already have classify_float (or x = x).

gasche Feb 28, 2018

Member

I'm happy to see more orders exposed and documented, but there is no doubt in my mind that (Float.compare x y) must have the exact same semantics as (Pervasives.compare x y) when x, y are floats.

alainfrisch reviewed Feb 27, 2018

 *) val equal: t -> t -> bool (** The equal function for floating-point numbers. *)

alainfrisch Feb 27, 2018

Contributor

If would be useful to document where this is compare x y = 0 or x = y.

alainfrisch reviewed Feb 27, 2018

 @@ -555,14 +555,14 @@ bytecomp/emitcode.cmo : bytecomp/translmod.cmi typing/primitive.cmi \ bytecomp/opcodes.cmo utils/misc.cmi bytecomp/meta.cmi \ parsing/location.cmi bytecomp/lambda.cmi bytecomp/instruct.cmi \ typing/ident.cmi typing/env.cmi utils/config.cmi bytecomp/cmo_format.cmi \ utils/clflags.cmi typing/btype.cmi parsing/asttypes.cmi \ bytecomp/emitcode.cmi utils/clflags.cmi bytecomp/bytegen.cmi typing/btype.cmi \

alainfrisch Feb 27, 2018

Contributor

(Just to be sure: I assume this change is unrelated to this PR, right?)

nojb Feb 27, 2018

Author Contributor

Yes, I guess it is a side-effect of make depend.

yallop reviewed Feb 27, 2018

 external modf : float -> float * float = "caml_modf_float" type t = float external compare : float -> float -> int = "%compare" let equal x y = compare x y = 0

yallop Feb 27, 2018

Member

Float.equal and Pervasives.(=) will behave differently, since

# nan = nan;;
- : bool = false
# Pervasives.compare nan nan;;
- : int = 0


If that's the desired behaviour it'd be good to have a comment in the documentation for Float.equal

Contributor

xclerc commented Feb 27, 2018

 Would it make sense to add functions for conversion to/from bits? (Int{32,64}.{bits_of_float,float_of_bits}) And also maybe conversion to/from integer types ({Nativeint,Int32,Int64}.{of,to}_float).
Contributor Author

nojb commented Feb 27, 2018

 Would it make sense to add functions for conversion to/from bits? (Int{32,64}.{bits_of_float,float_of_bits}) And also maybe conversion to/from integer types ({Nativeint,Int32,Int64}.{of,to}_float). Possibly, but in the existing numeric modules the conversion functions are only put in one of the sides, not in both (e.g. there is no Int32.to_int64), so this is a change that could be the subject of a different PR and that would affect existing numeric modules as well.
Contributor

xclerc commented Feb 27, 2018

 Sorry, it wasn't clear, I was actually suggesting to only add thing to the new Float module: Float.to_bits32 : t -> int32; Float.of_bits32 : int32 -> t; Float.to_bits64 : t -> int64; Float.of_bits64 : int64 -> t; Float.to_int32 : t -> int32; Float.of_int32 : int32 -> t; Float.to_int64 : t -> int64; Float.of_int64 : int64 -> t; Float.to_nativeint : t -> nativeint; Float.of_nativeint : nativeint -> t.

Contributor

alainfrisch commented Feb 27, 2018

 Cf also #1354.

Contributor

alainfrisch commented Feb 28, 2018

 So many nice other PRs depends on this new Float module; we don't want to delay it too much. So let's keep the discussed additions (conversions with int32/int64; other ordering/equality; etc) for later, and focus instead on polishing the existing PR (documentation, dropping dependency between Float and Array, etc).

nojbforce-pushed the nojb:float_module branch from 26c61ef to 8a80237Feb 28, 2018

Contributor Author

nojb commented Feb 28, 2018

 I removed the dependency between Array and Float, added a note to the doc of Float.equal specifying that it uses compare as comparison function and added Float.hash. I also rebased to avoid cluttering the commit history with make depend and bootstrapping commits.

alainfrisch reviewed Feb 28, 2018

 @@ -85,6 +85,8 @@ external modf : float -> float * float = "caml_modf_float" type t = float external compare : float -> float -> int = "%compare" let equal x y = compare x y = 0 external seeded_hash_param : int -> int -> int -> float -> int = "caml_hash" [@@noalloc] let hash x = seeded_hash_param 10 100 0 x

alainfrisch Feb 28, 2018

Contributor

(Later, it could be useful to create an ad hoc runtime primitive for Float.hash, with an unboxed form.)

alainfrisch reviewed Feb 28, 2018

 (* *) (* OCaml *) (* *) (* Xavier Leroy, projet Cristal, INRIA Rocquencourt *)

Contributor

alainfrisch reviewed Feb 28, 2018

 val to_string : float -> string (** Return the string representation of a floating-point number. *) type fpclass = Pervasives.fpclass =

alainfrisch Feb 28, 2018

Contributor

Shouldn't this be Stdlib.fpclass?

nojb Feb 28, 2018

Author Contributor

I tried, but got into trouble with ocamldoc:

../../byterun/ocamlrun ../../ocamlc -nostdlib -nopervasives -I  -c -open Pervasives float.mli
File "float.mli", line 109, characters 15-29:
Error: Unbound module Stdlib
Makefile:17: recipe for target 'float.cmi' failed
make[4]: *** [float.cmi] Error 2
make[4]: Leaving directory '/home/nojebar/ocaml/ocamldoc/stdlib_non_prefixed'
Makefile.unprefix:109: recipe for target '../ocamldoc/stdlib_non_prefixed/pervasives.cmi' failed
make[3]: *** [../ocamldoc/stdlib_non_prefixed/pervasives.cmi] Error 2


Any ideas how to fix this? @diml ?

diml Feb 28, 2018

Member

The -I -c is odd, it might be because of -I $(HERE) in the Makefile, not sure where this is defined. @Octachron I believe you wrote this line, do you know where $(HERE) is defined?

Octachron Feb 28, 2018 • edited

Contributor

Nowhere. The whole flag -I \$(HERE) should be removed. Nevertheless, this does not affect the issue at hand: ocamldoc simply knows nothing about a Stdlib when building the documentation and is only aware of the extracted Pervasives module. Moreover, ocamldoc limited support of module type of means that it would not be able to link to Stdlib.fpclass even after building the Stdlib documentation.

diml Feb 28, 2018

Member

Hmm, indeed. I don't see a simple solution to this problem, we could sed s/Stdlib/Pervasives/ when copying the mli files. Thought it's slightly not satisfactory that the documentation will mention pervasives rather than stdlib

alainfrisch Feb 28, 2018

Contributor

Ok, so we keep Pervasives, and people who insist on deprecating this module will find a solution ;)

Contributor

alainfrisch commented Feb 28, 2018

 LGTM (except perhaps the minor comment about Pervasives vs Stdlib). We can always add components or refine the doc later. I'll merge soon if nobody objects.
Contributor

Octachron commented Feb 28, 2018

 A minor point: the new module is missing an entry in the documentation manual/manual/libref/stdlib.etex, should I take care of this point after the merge?
Contributor Author

nojb commented Feb 28, 2018

 A minor point: the new module is missing an entry in the documentation manual/manual/libref/stdlib.etex, should I take care of this point after the merge? I will add one, thanks!

Octachron reviewed Feb 28, 2018

 @@ -62,6 +62,7 @@ from being garbage-collected \\ \subsubsection*{Arithmetic:} \begin{tabular}{lll} "Complex" & p.~\pageref{Complex} & Complex numbers \\ "Float" & p.~\pageref{Float} & Floating-point numbers \\

Octachron Feb 28, 2018

Contributor

You missed the index part starting from line 100 for the html version of the manual, and line 147 for the latex version.

nojb Feb 28, 2018

Author Contributor

Oops, thanks! Should be fixed now.

Octachron Feb 28, 2018

Contributor

The manual looks good now, thanks!

Chris00 Mar 1, 2018

Member

Shouldn't it be explicitly said that it is Double precision floating-point numbers?

nojb Mar 1, 2018

Author Contributor

Maybe. In http://caml.inria.fr/pub/docs/manual-ocaml/core.html#sec547, type float is described as being the type of "floating-point numbers" though.

Chris00 Mar 1, 2018

Member

Sure but there is a chance that people find the module Float before the type float.

Contributor

dbuenzli left a comment

 It seems that the PR left adding interesting constants like pi or e, also signature-wise it could be made more IntX like (e.g. adding zero, one).
 floating-point number greater than [1.0]. *) external of_int : int -> float = "%floatofint" (** Convert an integer to floating-point. *)

dbuenzli Feb 28, 2018

Contributor

I think it would be nice to add a constant max_exact_int and indicate that integers in the range [-max_exact_int;max_exact_int] will be represented exactly. This constant is also useful e.g. when you serialize integers to json numbers.

alainfrisch Mar 1, 2018

Contributor

I agree, but perhaps for a future addition. As simple as such an addition seems, there will be space for discussion, such as:

• The suggested naming is confusing since of course this is not the maximum integer represented as a float (that would be max_float).

• What would be the type of this value? float? int? int64?

dbuenzli Mar 1, 2018

Contributor

Yes I was not convinced by the name I gave in Gg.Float and tried max_exact_int which is silly. Regarding the type of the value it can't be int (which can be 31-bits) and I don't really see why this should be an int64, it's a particular value of that type, you can convert it to something else if you need.

Contributor

alainfrisch commented Mar 1, 2018

 It seems that the PR left adding interesting constants like pi or e, also signature-wise it could be made more IntX like (e.g. adding zero, one). Indeed. About pi, there is #964. The goal here is to make Float enter Stdlib rather quickly, without real additions compared to existing features (to make the PR consensual) in order to unlock many other PRs that are pending on it.

alainfrisch reviewed Mar 1, 2018

 val min_float : float (** The smallest positive, non-zero, non-denormalized value of type [float]. *) val epsilon_float : float

alainfrisch Mar 1, 2018

Contributor

About the naming of these constants:

• Should that be just named epsilon?

• For max_float/min_float, one could use max_value (and later add Int32.max_value as an alias for Int32.max_int, etc).

nojb Mar 1, 2018

Author Contributor

I don't have a strong opinion on epsilon_float. For the others, I would leave it for a different PR that addresses all numeric modules at once.

alainfrisch Mar 1, 2018

Contributor

(Also, min_value is really a bad name for min_float.)

Chris00 Mar 1, 2018 • edited

Member

I also think it should simply be epsilon.

Chris00 Mar 1, 2018

Member

What about smallest or lowest or the more explicit smallest_pos instead of min_float?

nojb Mar 1, 2018

Author Contributor

OK, I renamed it epsilon.

mookid Mar 3, 2018

Contributor

a nice thing about epsilon_float is the similarity with float.h (where it is DBL_EPSILON).

Chris00 Mar 4, 2018

Member

Float.epsilon is as similar — moreover, I do not think we aim to mimic C.

Chris00 reviewed Mar 1, 2018

 external div : float -> float -> float = "%divfloat" (** Floating-point division. *)

Chris00 Mar 1, 2018

Member

I think we should also add prefix (~+, ~-) and infix operators (+., -., *., /., **). The idea is that if these operators are redefined by another module, one can locally open this one to recover the usual behavior.

nojb Mar 1, 2018

Author Contributor

Definitely something that needs to be considered - but I think it is better to do it in a different PR that addresses all numeric modules at the same time.

BTW, wouldn't it make sense to use (+) instead of (+.) now that we have a separate module for it ?

Chris00 Mar 4, 2018

Member

We could but it would make heavier to mix integer and floating points in the same expression — a situation that occurs quite often.

Chris00 reviewed Mar 1, 2018

 ordering relation. *) val equal: t -> t -> bool (** The equal function for floating-point numbers, compared using {!compare}. *)

Chris00 Mar 1, 2018

Member

I think specialized versions of min and max should be provided (clearly describing their behavior on NaN — maybe several versions are required: one that ignores NaN, another one that returns NaN as soon as one is present).

xavierleroy Mar 2, 2018

Contributor

The documentation for [equal] is obscure. What about:

(** [equal x y] compares [x] and [y] for equality.  Unlike standard equality on floating-point numbers, [equal] treats [nan] as equal to itself and different from any other floating-point value.  This treatment of [nan] ensures that [equal] defines an equivalence relation.  [equal x y] is equivalent to [compare x y = 0].  *)

Member

Chris00 commented Mar 12, 2018

 I would drop the f since this is due to the poor man module system (hum) of C. For classify_float, I would prefer Float.classify.

Chris00 reviewed Mar 12, 2018

 external get : t -> int -> float = "%floatarray_safe_get" external set : t -> int -> float -> unit = "%floatarray_safe_set" external unsafe_get : t -> int -> float = "%floatarray_unsafe_get" external unsafe_set : t -> int -> float -> unit = "%floatarray_unsafe_set"

Chris00 Mar 12, 2018

Member

Shouldn't there be more operations such as blit, append,...?

alainfrisch Mar 12, 2018

Contributor

The current version is just a copy of the previous Array.Floatarray. We can extend it later.

bluddy commented Mar 12, 2018

 I would drop the f since this is due to the poor man module system (hum) of C. For classify_float, I would prefer Float.classify. Exactly. Trying to match C is silly -- they're dealing with a global namespace. The whole point here is that people can automatically guess the names of functions they need without looking them up because they're the same name as non-float functions, except in the Float module. They need to think about what the function does -- not about arcane prefixes or suffixes.
Contributor

alainfrisch commented Mar 12, 2018

 I suggest the following set of changes: classify_float -> classify drop the prefixes for Float.fpclass (Normal|Subnormal|Zero|Infinite|Nan) drop modf: the naming is not so clear, and one might want to return a record with 2 (unboxed) floats instead for performance reasons; let's keep that for later drop rem/fmod, unless a consensus emerges on the naming
Contributor

alainfrisch commented Mar 12, 2018

 The current state LGTM. Does anyone want to suggest other changes? (Additions can be left for later.)

Contributor

alainfrisch commented Mar 15, 2018

 @nojb Let's merge this one! Can you rebase/fix the conflicts?

nojb added some commits Feb 23, 2018

 Add Float module 
 7f6d059 
 make depend 
 242b6f2 
 Changes 
 6f8419e 
 Add Float.Array module 
 8b396af 
 Add Float.hash 
 8bb6d26 
 Add manual entry 
 b277337 
 Copyright 
 a36e1e7 
 Add missing manual entries 
 94dc90d 
 Rename epsilon_float -> epsilon 
 2f6ffb1 
 Remove unwanted diff 
 5833954 

nojbforce-pushed the nojb:float_module branch from 0005044 to 090a65dMar 15, 2018

Contributor Author

nojb commented Mar 15, 2018

 Can you rebase/fix the conflicts? Done.
Contributor

alainfrisch commented Mar 15, 2018

 Ok, hopefully the CI will finish before the next conflict pops up.
Contributor

trefis commented Mar 15, 2018

 Just out of curiosity: why is a bootstrap needed for this change?

nojbforce-pushed the nojb:float_module branch from 090a65d to 5833954Mar 15, 2018

Contributor Author

nojb commented Mar 15, 2018

 Just out of curiosity: why is a bootstrap needed for this change? Good catch, it is not! I guess it was needed at some intermediate stage of the PR (or at least I thought it was) and it was left there after that. I have removed the bootstrap commit.

nojb merged commit 4724e39 into ocaml:trunk Mar 16, 2018 2 checks passed

2 checks passed

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

Merged