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

New methods and functions in Typed_array #970

Merged
merged 7 commits into from
Mar 18, 2020
Merged

New methods and functions in Typed_array #970

merged 7 commits into from
Mar 18, 2020

Conversation

Ngoguey42
Copy link
Contributor

@Ngoguey42 Ngoguey42 commented Feb 26, 2020

Follow-up on #969

  • Add getInt16 do dataView
  • Add slice to typedArray and arrayBuffer
  • Add slice_toEnd to typedArray and arrayBuffer
  • Add *fromGenarray and toGenarray functions

@hhugo
Copy link
Member

hhugo commented Mar 5, 2020

Thanks for your contribution.
I wonder if we should organize and name theses new functions a bit differently.
Also, do you have any preference between Genarray and Array1 ? I suspect Array1 will be more convenient to use, but you might have a different use-case in mind ..

What do you think about introducing a new sub module like

module Bigarray : sig
  val of_int8_signed : (int, int8_signed_elt, c_layout) Bigarray.Array1.t -> int8Array t
  val to_int8_signed : int8Array t -> (int, int8_signed_elt, c_layout) Bigarray.Array1.t
  val of_int8_unsinged : (int, int8_unsigned_elt, c_layout) Bigarray.Array1.t -> int8Array t
  val to_int8_unsigned : int8Array t -> (int, int8_unsigned_elt, c_layout) Bigarray.Array1.t
  ...
end

@Ngoguey42
Copy link
Contributor Author

Ngoguey42 commented Mar 12, 2020

Array1 instead of Genarray, one or the other I don't really mind. A Genarray.t has the advantage of being also the ndarray in Owl. Your call.

About the naming, since the Bigarray module is all about GADT i figured it was possible to create two generic from_genarray to_genarray functions. In my latest commit I have pushed an implementation of it and some unit tests. It implies some breaking changes.

In the implementation i had to:

  • get rid of the _content_type_ method that seem unused,
  • remove ##set_fromArray and replace it with ##set_fromIntArray and ##set_fromFloatArray,
  • use locally abstract type in the set/get/unsafe_get methods. When those are used with a int32Array or a uint32Array: int32 is the scalar type instead of int.

I can always roll back to the previous implementation if needed.

@hhugo
Copy link
Member

hhugo commented Mar 12, 2020

Thanks.
I would prefer to not extend the scope of this PR too much.
Can you revert the three following changes

In the implementation i had to:

  • get rid of the _content_type_ method that seem unused,
  • remove ##set_fromArray and replace it with ##set_fromIntArray and ##set_fromFloatArray,
  • use locally abstract type in the set/get/unsafe_get methods. When those are used with a
    int32Array or a uint32Array: int32 is the scalar type instead of int.

@Ngoguey42
Copy link
Contributor Author

Okey. Should the former bigarray/typed_array conversions still be included in that PR? They would not make sense alongside a polymorphic implementation introduced in another PR.

@hhugo
Copy link
Member

hhugo commented Mar 12, 2020

Why is that ? I think I'm missing something ..

@Ngoguey42
Copy link
Contributor Author

In the latest commit (1a8b1d2), I was able to replace the 14 functions (int8Array_fromGenarray, int8Array_toGenarray, int16Array_fromGenarray, etc....) by those two: from_genarray and to_genarray.

Those 3 changes are mandatory for the from_genarray/to_genarray implementation

  • get rid of the _content_type_ method that seem unused,
  • remove ##set_fromArray and replace it with ##set_fromIntArray and ##set_fromFloatArray,
  • use locally abstract type in the set/get/unsafe_get methods. When those are used with a int32Array or a uint32Array: int32 is the scalar type instead of int.

Reverting those 3 changes means bringing back the 14 previous functions.

Sorry if I wasn't clear.

@hhugo
Copy link
Member

hhugo commented Mar 12, 2020

I don't understand/see why there are mandatory

@hhugo
Copy link
Member

hhugo commented Mar 12, 2020

To be clear, I'd like to keep from_genarray/to_genarray in this PR and don't see why the 3 other changes are mandatory consequences.

@Ngoguey42
Copy link
Contributor Author

In method _content_type_ : 'b, 'b was the polymorphic variant type that is now gone in that implementation.


On master the 'a of class type ['a, 'b] typedArray is either an int or a float. It is actually used in only those 4 functions:

method set_fromArray : 'a js_array t -> int -> unit meth
val set : ('a, 'b) typedArray t -> int -> 'a -> unit
val get : ('a, 'b) typedArray t -> int -> 'a optdef
val unsafe_get : ('a, 'b) typedArray t -> int -> 'a

In order to be compatible with Bigarray,

type int32Array = (int, [ `Int32 ]) typedArray
type uint32Array = (float, [ `Uint32 ]) typedArray

should now be:

type int32Array = (int32, Bigarray.int32_elt) typedArray
type uint32Array = (int32, Bigarray.int32_elt) typedArray

It means that 'a can now also be an int32, but the 4 functions above are not compatible with it and should be changed.

I chose to replace the those 4 functions with 8 non-polymorphic ones (set_fromIntArray/set_float/etc...), and I could mostly ensure the backward compatibility of set/get/unsafe_get using locally abstract types.

@hhugo
Copy link
Member

hhugo commented Mar 12, 2020

but the 4 functions above are not compatible

I'm still missing something, why would int32 be problematic ?

@Ngoguey42
Copy link
Contributor Author

You are right, it works like a charm. It wrongly expected the int32 to be an opaque ocaml object, but I just discovered that it is implemented as a javascript number, like int and float. I just reverted those 4 functions.

lib/js_of_ocaml/typed_array.ml Outdated Show resolved Hide resolved
lib/js_of_ocaml/typed_array.ml Show resolved Hide resolved
@hhugo
Copy link
Member

hhugo commented Mar 12, 2020

close #971

@hhugo
Copy link
Member

hhugo commented Mar 12, 2020

One issue is that we can now call set_fromTypedArray mixing uint32 and int32 typed array.
But it seems fine.

@hhugo hhugo merged commit df86d2c into ocsigen:master Mar 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants