owlbarn / owl

Owl - OCaml Scientific Computing @ https://ocaml.xyz
MIT License
1.21k stars 122 forks source link

Invalid_argument("index out of bounds") with Algodiff #623

Open talex5 opened 2 years ago

talex5 commented 2 years ago

This program adds two arrays together using Algodiff:

open Owl

module AD = Owl.Algodiff.D

let a = AD.Arr (Arr.zeros [| 1; 1 |])
let b = AD.make_reverse (AD.Arr (Arr.zeros [| 1 |])) (AD.tag ())

let () =
  let x = AD.Maths.(a + b) in
  AD.Mat.print x;
  AD.reverse_prop (F 1.) x;
  AD.Mat.print (AD.adjval b)

It fails at the reverse_prop step with:

   C0 
R0  0 
Fatal error: exception Invalid_argument("index out of bounds")
Raised by primitive operation at Owl_utils_array.sort_fill in file "src/base/misc/owl_utils_array.ml", line 483, characters 17-22
Called from Owl_utils.squeeze_continuous_dims in file "src/base/misc/owl_utils.ml", line 166, characters 13-67
Called from Owl_dense_ndarray_generic.sum_reduce in file "src/owl/dense/owl_dense_ndarray_generic.ml", line 10117, characters 16-59
Called from Owl_algodiff_ops.Make.Maths._sum_reduce.(fun).ff_arr in file "src/base/algodiff/owl_algodiff_ops.ml", line 584, characters 36-54
Called from Owl_algodiff_ops_builder.Make.build_piso.(fun).f.r_c_d.adjoint in file "src/base/algodiff/owl_algodiff_ops_builder.ml", line 364, characters 37-66
Called from Owl_algodiff_reverse.Make.reverse_push.push in file "src/base/algodiff/owl_algodiff_reverse.ml", line 42, characters 20-37
Called from Owl_algodiff_reverse.Make.reverse_prop in file "src/base/algodiff/owl_algodiff_reverse.ml" (inlined), line 53, characters 4-20
Called from Dune__exe__Test in file "test.ml", line 11, characters 2-26

I assume that since the forwards part works, the reverse step should work too. I guess it's something to do with the shape of b. If I use [|1; 1|] then it works.

tachukao commented 2 years ago

I'm not entirely sure if this is the issue, but I think it might have to do with F 1.not being the same shape as x. I would try using an array of ones instead of F 1..

talex5 commented 2 years ago

I would try using an array of ones instead of F 1.

That doesn't help:

utop # #require "owl-top";;
utop # open Owl

module AD = Owl.Algodiff.D

let a = AD.Arr (Arr.zeros [| 1; 1 |])
let b = AD.make_reverse (AD.Arr (Arr.zeros [| 1 |])) (AD.tag ())

let () =
  let x = AD.Maths.(a + b) in
  AD.Mat.print x;
  AD.reverse_prop (Arr (Arr.ones [| 1 |])) x;
  AD.Mat.print (AD.adjval b);;

   C0 
R0  0 
Exception: Invalid_argument "index out of bounds".

Anyway, I can fix this myself by extending b. I'm just reporting it in case you want to fix it.

mseri commented 2 years ago

By mistake, I just found out that it does not fail the second time... Try this

try AD.reverse_prop (Arr (Arr.ones [| 1 |])) x with e -> AD.reverse_prop (Arr (Arr.ones [| 1 |])) x;

or

try AD.reverse_prop (F 1.) x with e -> AD.reverse_prop (F 1.) x;

and it will return unit fine 😮