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 Stdlib.Float.Array #1936

Merged
merged 10 commits into from Dec 3, 2018

Conversation

Projects
None yet
7 participants
@damiendoligez
Copy link
Member

commented Jul 27, 2018

Work in progress:

  • need to add tests
  • need to discuss introducing a Float -> List dependency
  • maybe some more functions should be added
@xclerc

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2018

For the sake of consistency, I would suggest to consider
the addition of a labelled version.

@alainfrisch alainfrisch added the stdlib label Jul 30, 2018

@damiendoligez damiendoligez force-pushed the damiendoligez:add-stdlib-float-array branch from a38a823 to 2057408 Oct 1, 2018

@damiendoligez damiendoligez force-pushed the damiendoligez:add-stdlib-float-array branch from 2057408 to 87348ff Oct 10, 2018

@damiendoligez

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2018

I've added the labeled version, I've added tests (which revealed a bug in concat) and I've decided that more functions can be added in a later PR.

This is now ready for review.

Two points that may need discussion:

  • the dependency on List introduced by this PR
  • the reuse of memq with a different (but related) meaning

@xclerc, @alainfrisch would you like to review?


val memq : float -> t -> bool
(** Same as {!mem}, but uses float equality instead of generic
equality to compare elements. *)

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Oct 10, 2018

Contributor

The doc for mem and memq is confusing (or it is me being confused, perhaps). "float equality" could be interpreted as Float.equal, which behaves as the generic comparison (i.e. Stdlib.compare x y = 0), but memq rather uses the generic equality (i.e. (=)).

This comment has been minimized.

Copy link
@mshinwell

mshinwell Oct 11, 2018

Contributor

I'm not sure we're in a good state at the moment with regard to IEEE semantics on float equality. I tend to think that it's a mistake that Float.equal does not have the IEEE semantics---my guess is that users would expect it to have such semantics (I certainly did), and indeed the polymorphic equality operator has such semantics. I wonder if we should change Float.equal and add a second Float.equal_non_ieee or similar. I know this means that equal functions cannot in general be defined as compare x y = 0, but I think that is an inevitable consequence if assuming IEEE semantics globally.

This comment has been minimized.

Copy link
@yallop

yallop Oct 29, 2018

Member

It looks as though Array.(memq x (make 1 x)) is always false for floats, but Float.Array.(memq x (make 1 x)) is true (for non-NaNs).

That seems rather surprising, and I wonder if it'd be better to pick a different name. Alternatively, perhaps just drop memq altogether, since memq x is just exists ((=)x) in any case.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2018

I'll review that in more details later, but one high-level suggestion would be to use this PR to start thinking about a generic ARRAY signature such that Float.Array implements ARRAY with type elt = float and type t = floatarray. Typically, one will want to have Bytes implement ARRAY with type elt = char and type t = bytes, which could create some constraints in terms of naming values.

(Also, it could make sense to make sure that one could map between two specialized array types, without going through generic arrays (perhaps by passing the implementations as first class modules).)

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2018

@alainfrisch I agree there should be a standard signature, although I would prefer to see "Array_intf.S" rather than the rather shouty retro-SML "ARRAY".

Show resolved Hide resolved stdlib/float.mli Outdated
Show resolved Hide resolved stdlib/float.mli Outdated
Raise [Invalid_argument] if [n < 0] or [n > Sys.max_floatarray_length]. *)

val create : int -> t

This comment has been minimized.

Copy link
@xclerc

xclerc Oct 11, 2018

Contributor

It feels a bit odd to have Array.create deprecated
while Float.Array.create is not. (That said, I reckon
that marking a function just introduced as deprecated
might feel equally weird.)

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Oct 29, 2018

Author Member

The general idea with make and create is that make builds initialized data structures while create allocates uninitialized data.

In this light, Array.create should not exist while Float.Array.create is a valid operation.

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Oct 29, 2018

Contributor

(That said, allocating an uninitialized array should be somehow discouraged for casual use (perhaps just with a disclaimer in the docstring), for evident reasons (non-reproducibility, risk of leaking sensitive information, etc). The same applies to Bytes.create of course.)

This comment has been minimized.

Copy link
@xclerc

xclerc Nov 8, 2018

Contributor

Understood.

Raise [Invalid_argument] if [n < 0] or [n > Sys.max_floatarray_length]. *)

(* create_float *)

This comment has been minimized.

Copy link
@xclerc

xclerc Oct 11, 2018

Contributor

Not having create_float seems natural, but it means
that Array and Float.Array will not share the very same
signature. (Anyway, this comment should probably go.)

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Oct 29, 2018

Author Member

Array and Float.Array cannot have the same signature anyway because in one case t is a type constructor and in the other it's a simple type.

This comment has been minimized.

Copy link
@xclerc

xclerc Nov 8, 2018

Contributor

Indeed

Raise [Invalid_argument] if [n < 0] or [n > Sys.max_floatarray_length]. *)

(* make_matrix *)

This comment has been minimized.

Copy link
@xclerc

xclerc Oct 11, 2018

Contributor

(Same issue.)

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Oct 29, 2018

Author Member

While we could have create_float (but it would be slightly weird), we can't have arrays of arrays in Float.Array so make_matrix cannot exist.

This comment has been minimized.

Copy link
@xclerc

xclerc Nov 8, 2018

Contributor

(I was more or less expecting val make_matrix : int -> int -> float -> t array,
but I reckon it might be more confusing than useful.)

List.init (length a) (unsafe_get a)

let of_list l =
let result = create (List.length l) in

This comment has been minimized.

Copy link
@xclerc

xclerc Oct 11, 2018

Contributor

(It is a bit of a stretch but contrary to concat,
we do not protect against overflow during length
computation.)

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Oct 29, 2018

Author Member

Unlike the concat case, List.length cannot overflow.

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Oct 29, 2018

Contributor

It could with js_of_ocaml, no?

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Nov 5, 2018

Author Member

With 32-bit ints on a 64-bit machine, it seems so. Then we have the same problem in array.ml

Show resolved Hide resolved stdlib/float.ml
let iter f a =
for i = 0 to length a - 1 do f (unsafe_get a i) done

let iter2 f a b =

This comment has been minimized.

Copy link
@xclerc

xclerc Oct 11, 2018

Contributor

(Same issue.)

Show resolved Hide resolved stdlib/float.ml
in
aux 0

(* mostly duplicated from array.ml *)

This comment has been minimized.

Copy link
@xclerc

xclerc Oct 11, 2018

Contributor

(The "mostly" is related to the special casing of
the empty array. Shall we keep the special casing
by introducing a constant?)

This comment has been minimized.

Copy link
@xclerc

xclerc Oct 11, 2018

Contributor

(Would also apply to map, map2, and mapi.)

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Oct 29, 2018

Author Member

There's also the create that takes only one argument.

This comment has been minimized.

Copy link
@xclerc

xclerc Nov 8, 2018

Contributor

Sorry, I meant "mostly" because the version from Array
starts with: function [] -> [||] | .... So, I wondered
whether it would make sens to have a similar fast track
here, possibly by defining let empty = create 0.

@xclerc

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2018

I have made a first slightly-superficial review (e.g. did not
read the tests yet) in order to give early feedback.

I would second the introduction of Array_intf.S, and if
there is no consensus at least introduce such a signature
in the testsuite in order to enforce consistency through
tests.

Also, it looks like it cannot be part of the review because
the code is untouched, but I would suggest to mark the
Array.Floatarray module as deprecated (even though
it is currently "undocumented").

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2018

I would prefer to see "Array_intf.S" rather than the rather shouty retro-SML "ARRAY".

Array_intf.S also does not look very stdlib-ish. What about Array.S?

@xclerc

This comment has been minimized.

Copy link
Contributor

commented Oct 12, 2018

(Well, module name aside, I think it makes sense to have
the signature outside of Array since its very purpose is
to be shared by multiple structures.)

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2018

(Well, module name aside, I think it makes sense to have
the signature outside of Array since its very purpose is
to be shared by multiple structures.)

Why would defining the signature inside Array make it more difficult to share it by multiple structures?

Note that Array itself would not satisfy this signature (because of the type parameter).

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2018

(But Array should probably expose an Array.Make to produce monomorphic array structures compatible with Array.S.)

@xclerc

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2018

Why would defining the signature inside Array make it more difficult to share it by multiple structures?

It would certainly not be more difficult; it just felt asymmetrical.

@damiendoligez

This comment has been minimized.

Copy link
Member Author

commented Oct 29, 2018

If we define a signature, it cannot contain make_float (which is a hack, so that's probably OK) and make_matrix (more annoying).

@damiendoligez damiendoligez changed the title [WIP] add Stdlib.Float.Array add Stdlib.Float.Array Oct 29, 2018

@xclerc

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2018

After re-reading, the whole thing, I would have
some minor clearly-non-blocking comments:

  • the documentation mentions -flat-float-array
    (configure-time option), but I understand the name
    might change due to @shindere 's work on autoconf;
  • the tests could use more [nan] occurrences
    (for instance in comparison/sorting tests);
  • the tests might benefit from a parallel execution
    of operations on Float.Array.t and float array
    values. The Array module has been "proved"
    correct by the test of time, so we could leverage
    that to test the new module (and ensure that
    implementations will not diverge at some point).
@xclerc

This comment has been minimized.

Copy link
Contributor

commented Nov 19, 2018

ping @damiendoligez

It looks like the only important point to discuss is #1936 (comment).

@damiendoligez damiendoligez force-pushed the damiendoligez:add-stdlib-float-array branch from 969fa4b to 04fc78f Nov 28, 2018

@damiendoligez

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2018

I love indian food, so I added some nans. Also infinities.
I now run the same tests on both Float.Array.t and float array.
Following offline discussions with @mshinwell and @lpw25 I renamed memq to mem_ieee.
I clarified the documentation of Array.mem because "structural equality" might mean = or compare but they are not the same, especially with respect to nan.

@xclerc

This comment has been minimized.

Copy link
Contributor

commented Nov 30, 2018

That is great; thanks! (I always forget infinities, probably because
I find them far less confusing...).

(Regarding the last item on your todo list, I would say it is not really
a problem since you actually depend only on List.{init,length}.
So, if the dependency on List happens to be a problem, I guess
it will be acceptable to just copy these small functions to float.ml.)

@damiendoligez

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2018

@alainfrisch @xclerc Can I get this formally approved?

@xclerc

xclerc approved these changes Nov 30, 2018

@damiendoligez damiendoligez merged commit fc60f71 into ocaml:trunk Dec 3, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Member Author

commented Dec 3, 2018

Thanks !

@damiendoligez damiendoligez deleted the damiendoligez:add-stdlib-float-array branch Dec 3, 2018

@gasche

This comment has been minimized.

Copy link
Member

commented Dec 3, 2018

Merging this PR broke the post-merge CI, with the following log:

Running tests from 'tests/lib-floatarray' ...
 ... testing 'floatarray.ml' with 1 (native) => failed (Running program /home/ci/builds/workspace/main/flambda/true/label/ocaml-ubuntu-latest/testsuite/_ocamltest/tests/lib-floatarray/floatarray/ocamlopt.byte/floatarray.opt without any argument: command
/home/ci/builds/workspace/main/flambda/true/label/ocaml-ubuntu-latest/testsuite/_ocamltest/tests/lib-floatarray/floatarray/ocamlopt.byte/floatarray.opt 
failed with exit code 2)
 ... testing 'floatarray.ml' with 2 (bytecode) => passed

gretay-js added a commit to gretay-js/ocaml that referenced this pull request Dec 3, 2018

add Stdlib.Float.Array (ocaml#1936)
Add Stdlib.Float.Array module with a bunch of functions for the
floatarray type.
@shindere

This comment has been minimized.

Copy link
Contributor

commented Dec 3, 2018

To be more precise, it's an assertion failure at line 208 of the test
program, which says:

  assert (A.of_list (A.to_list a) = a);
@gasche

This comment has been minimized.

Copy link
Member

commented Dec 5, 2018

@damiendoligez or someone else, could you fix the test failure? We need our CI to be working properly, so I will revert this PR by the end of the week if the issue is not fixed.

@xclerc

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2018

Indeed, the assertion pointed by @shindere will fail
if the uninitialized array contains a nan value.

@shindere

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2018

@xclerc

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2018

If this is actually the cause, we may just replace
any nan with e.g. 0..

@damiendoligez

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2018

Fixed (commit bbfe903) by both

  • using an initialized array
  • using compare instead of =

Tested on precheck (build 166).

Apologies for committing this ugly test in the first place.

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.