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

set_fancy does not work for some arrays with certain Bigarray.kind types. #671

Open
zoj613 opened this issue Jun 21, 2024 · 5 comments
Open

Comments

@zoj613
Copy link

zoj613 commented Jun 21, 2024

When using Owl.Dense.Ndarray.Generic.set_fancy with arrays of kinds not Float32, Float64, Complex32 or Complex64, the operation fails with error message Exception: Failure "_ndarray_set_fancy: unsupported operation", even though the kind is a valid Bigarray.kind type.

It appears this exception is thrown by the snippet:

let _ndarray_set_fancy
: type a b c.
(a, b) kind
-> (a, b) owl_arr
-> (a, b) owl_arr
-> (int64, c) owl_arr
-> (int64, c) owl_arr
-> unit
= function
| Float32 -> owl_float32_ndarray_set_fancy
| Float64 -> owl_float64_ndarray_set_fancy
| Complex32 -> owl_complex32_ndarray_set_fancy
| Complex64 -> owl_complex64_ndarray_set_fancy
| _ -> failwith "_ndarray_set_fancy: unsupported operation"

I think it is very limiting to only support just 4 kinds when the set operation should work for any of the Bigarray.kind types.

Here is a minimal example to reproduce the error:

module Ndarray = Owl.Dense.Ndarray.Generic
let arr = Ndarray.create Bigarray.Int32 [|3;4;3|] 5l
let slice = Owl_types.[L [1; 2]; R []; I 0]
let x = Ndarray.create Bigarray.Int32 [|2; 4; 1|] 0l
 Ndarray.set_fancy slice arr x

I expected this simple operation to work but it throws a |Exception: Failure "_ndarray_set_fancy: unsupported operation". exception.

I would love for Owl to support more kinds as im using it to write an array library that needs to support more than the 4 basic kinds listed above.

Cc @mseri @jzstark

zoj613 pushed a commit to zoj613/zarr-ml that referenced this issue Jun 28, 2024
This commit implements basic functionality.
- Supported codecs include Sharding Indexed, Gzip, Crc32c, Transpose and
  Bytes.
- Supported stores include MemoryStore and FilesystemStore.
- Supported data types include Float32, Float64, Complex32 and
  Complex64. This limitation is simply a result of Owl limited
  array write functionality as reported here: owlbarn/owl#671
- All the core Zarr V3 specification details have been implemented.
zoj613 pushed a commit to zoj613/zarr-ml that referenced this issue Jun 28, 2024
This commit implements basic functionality.
- Supported codecs include Sharding Indexed, Gzip, Crc32c, Transpose and
  Bytes.
- Supported stores include MemoryStore and FilesystemStore.
- Supported data types include Float32, Float64, Complex32 and
  Complex64. This limitation is simply a result of Owl limited
  array write functionality as reported here: owlbarn/owl#671
- All the core Zarr V3 specification details have been implemented.
zoj613 added a commit to zoj613/zarr-ml that referenced this issue Jul 5, 2024
`Ndarray.set_fancy*` functions unfortunately don't work for array
kinds other than Float32, Float64, Complex32 and Complex64.
See: owlbarn/owl#671 . As a workaround
we manually set each coordinate one-at-time using the basic
set function which does not suffer from this bug. It is likely
much slower for large Zarr chunks but necessary for usability.
zoj613 added a commit to zoj613/zarr-ml that referenced this issue Jul 5, 2024
`Ndarray.set_fancy*` functions unfortunately don't work for array
kinds other than Float32, Float64, Complex32 and Complex64.
See: owlbarn/owl#671 . As a workaround
we manually set each coordinate one-at-time using the basic
set function which does not suffer from this bug. It is likely
much slower for large Zarr chunks but necessary for usability.
@zoj613
Copy link
Author

zoj613 commented Jul 5, 2024

I was able to implement a temporary workaround to this issue by using element-wise set. Basically:

  1. Convert the slice to a list of coordinates of the target array.
  2. Use Owl.Dense.Ndarray.Generic.set and List.iter2 to iterate over the list of coordinates and values and set them one at a time
 List.iter2
  (fun coord val ->
	Owl.Dense.Ndarray.Generic.set coord val) coord_list value_list

I would like to add that this issue also affects the transpose function as well. The offending lines are:

let _ndarray_transpose
: type a b.
(a, b) kind
-> (a, b) owl_arr
-> (a, b) owl_arr
-> (int64, int64_elt) owl_arr
-> (int64, int64_elt) owl_arr
-> unit
= function
| Float32 -> owl_float32_ndarray_transpose
| Float64 -> owl_float64_ndarray_transpose
| Complex32 -> owl_complex32_ndarray_transpose
| Complex64 -> owl_complex64_ndarray_transpose
| _ -> failwith "_ndarray_transpose: unsupported operation"

Unfortunately, I haven't been able to find a workaround for this

@zoj613
Copy link
Author

zoj613 commented Jul 21, 2024

UPDATE: So I managed to find a workaround for computing the transpose of any kind using Owl functions. Essentially, this involves converting between .Any and .Generic representations of an array. The functions that seem to not work on .Generic happen to work on .Any arrays. Here is an implementation of transpose that works for any Bigarray kind:

module Ndarray = Owl.Dense.Ndarray.Generic
module Anyarray = Owl.Dense.Ndarray.Any

let transpose ?axis x =
  let shape = Ndarray.shape x in
  let y = Anyarray.init_nd shape @@ fun c -> Ndarray.get x c in
  let y' = Anyarray.transpose ?axis y in
  Ndarray.init_nd (Ndarray.kind x) (Anyarray.shape y') @@ fun c ->
    Anyarray.get y' c

I suppose this workaround can be generalized to any of the Generic functions that are limited to only 4 bigarray kinds?

Cc @jzstark

@mseri
Copy link
Member

mseri commented Jul 22, 2024

Nice. The main downside is that this won't scale well since it involves copying everything twice, but at least would circumvent the current unnecessary limitation

@zoj613
Copy link
Author

zoj613 commented Jul 22, 2024

Nice. The main downside is that this won't scale well since it involves copying everything twice, but at least would circumvent the current unnecessary limitation

Perhaps it can be used only for the Bigarray kinds not currently supported by the Generic module, that's if correctness/usability is prioritized over performance.

I am very interested in getting Generic functions to work for any kind. What part of the codebase would I need to extend to support this functionality? Is it doable or too complicated for someone who doesn't have intimate knowledge of the codebase?

@jzstark
Copy link
Collaborator

jzstark commented Sep 30, 2024

Thanks a lot the investigation about using other types of Bigarray! Unfortunately, due to that the type system holds a central position in the design of Owl, adding even one new type such as Float16 or Int requires daunting work which is not available to us now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants