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

Support the '0 dimension case' for bigarrays #787

Merged
merged 4 commits into from Sep 1, 2016

Conversation

Projects
None yet
4 participants
@LaurentMazare
Contributor

LaurentMazare commented Aug 30, 2016

Handle the '0 dimension' case for bigarray in the same way other numerical libraries (e.g. numpy or tensorflow) do, which is that such a bigarray holds a scalar value.

It seems that relaxing the exceptions that currently prevent this case is enough to get this working.

Note that this PR doesn't include any support for the bigarray.{} syntax, not sure how worthy this is.
It also doesn't include a Bigarray.Array0 module but I'm happy to add one if there is any need for it.

To give some context I came along this when writing some tensorflow ocaml bindings and the current workaround is to have a wrapper around the bigarray module that handles this scalar case with a bigarray with one dimension of size one. However this isn't very neat as it requires the wrapper to store whether the embedded data is a scalar or not. It would be quite nicer to have this supported by the bigarray module directly.

LaurentMazare added some commits Aug 29, 2016

Support arrays with 0 dimensions.
Test reshaping.

Also adapt the documentation.

Also allow slice to produce arrays with 0 dimensions.

Update Changes
Show outdated Hide outdated otherlibs/bigarray/bigarray_stubs.c
@@ -1050,7 +1050,7 @@ CAMLprim value caml_ba_slice(value vb, value vind)
/* Check number of indices < number of dimensions of array */

This comment has been minimized.

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

Should be <= now.

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

Should be <= now.

This comment has been minimized.

@LaurentMazare

LaurentMazare Aug 31, 2016

Contributor

Good catch, this is fixed now.

@LaurentMazare

LaurentMazare Aug 31, 2016

Contributor

Good catch, this is fixed now.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Aug 30, 2016

Contributor

The request is well founded and the implementation rather straightforward and well tested. I'm in favor of merging if nobody is against.

Contributor

alainfrisch commented Aug 30, 2016

The request is well founded and the implementation rather straightforward and well tested. I'm in favor of merging if nobody is against.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@hcarty

This comment has been minimized.

Show comment
Hide comment
@hcarty

hcarty Aug 30, 2016

Contributor

Could something be added to the documentation so users realize this is available/possible?

Contributor

hcarty commented Aug 30, 2016

Could something be added to the documentation so users realize this is available/possible?

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Aug 30, 2016

Contributor

The ability to have dimension 0 is mentioned in bigarray.mli.

Contributor

alainfrisch commented Aug 30, 2016

The ability to have dimension 0 is mentioned in bigarray.mli.

@hcarty

This comment has been minimized.

Show comment
Hide comment
@hcarty

hcarty Aug 30, 2016

Contributor

Ah, I missed those lines.

I would like an Array0 module and Array1.slice to go with this. I have had projects in the past which would have benefited from an Array0.

Contributor

hcarty commented Aug 30, 2016

Ah, I missed those lines.

I would like an Array0 module and Array1.slice to go with this. I have had projects in the past which would have benefited from an Array0.

LaurentMazare added some commits Aug 31, 2016

@LaurentMazare

This comment has been minimized.

Show comment
Hide comment
@LaurentMazare

LaurentMazare Aug 31, 2016

Contributor

I've added an Array0 module as well as Array1.slice. It makes the change larger so let me know if you think it's not worth it.

Contributor

LaurentMazare commented Aug 31, 2016

I've added an Array0 module as well as Array1.slice. It makes the change larger so let me know if you think it's not worth it.

@hcarty

This comment has been minimized.

Show comment
Hide comment
@hcarty

hcarty Aug 31, 2016

Contributor

I think the addition of the module, slice function and genarray conversions make this PR much more useful. Thank you for adding them!

I looked over the code and like the change.

Contributor

hcarty commented Aug 31, 2016

I think the addition of the module, slice function and genarray conversions make this PR much more useful. Thank you for adding them!

I looked over the code and like the change.

@gasche gasche merged commit 079de96 into ocaml:trunk Sep 1, 2016

1 of 2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Sep 1, 2016

Member

I merged, thanks to everyone for the comments, and for the nice pull request.

Member

gasche commented Sep 1, 2016

I merged, thanks to everyone for the comments, and for the nice pull request.

@LaurentMazare

This comment has been minimized.

Show comment
Hide comment
@LaurentMazare

LaurentMazare Sep 1, 2016

Contributor

That was pretty fast! Thanks all.

Contributor

LaurentMazare commented Sep 1, 2016

That was pretty fast! Thanks all.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Sep 1, 2016

Member

Our internal continuous integration server just let me know that clang has a seemingly-legitimate complaint about the C code:

bigarray_stubs.c:205:16: error: comparison of unsigned expression < 0 is always false [-Werror,-Wtautological-compare]
  if (num_dims < 0 || num_dims > CAML_BA_MAX_NUM_DIMS)
      ~~~~~~~~ ^ ~
bigarray_stubs.c:1302:16: error: comparison of unsigned expression < 0 is always false [-Werror,-Wtautological-compare]
  if (num_dims < 0 || num_dims > CAML_BA_MAX_NUM_DIMS)
      ~~~~~~~~ ^ ~
2 errors generated.
Member

gasche commented Sep 1, 2016

Our internal continuous integration server just let me know that clang has a seemingly-legitimate complaint about the C code:

bigarray_stubs.c:205:16: error: comparison of unsigned expression < 0 is always false [-Werror,-Wtautological-compare]
  if (num_dims < 0 || num_dims > CAML_BA_MAX_NUM_DIMS)
      ~~~~~~~~ ^ ~
bigarray_stubs.c:1302:16: error: comparison of unsigned expression < 0 is always false [-Werror,-Wtautological-compare]
  if (num_dims < 0 || num_dims > CAML_BA_MAX_NUM_DIMS)
      ~~~~~~~~ ^ ~
2 errors generated.
@LaurentMazare

This comment has been minimized.

Show comment
Hide comment
@LaurentMazare

LaurentMazare Sep 1, 2016

Contributor

Indeed I didn't noticed that num_dims is unsigned, sorry about that. Should I create a new PR to fix this ? (removing the num_dims < 0 conditions)

Contributor

LaurentMazare commented Sep 1, 2016

Indeed I didn't noticed that num_dims is unsigned, sorry about that. Should I create a new PR to fix this ? (removing the num_dims < 0 conditions)

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Sep 1, 2016

Member

I will do the change, run the testsuite, and commit if it works.

Member

gasche commented Sep 1, 2016

I will do the change, run the testsuite, and commit if it works.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Sep 1, 2016

Member

Should be fixed in 014d941 .

Member

gasche commented Sep 1, 2016

Should be fixed in 014d941 .

@nilsbecker nilsbecker referenced this pull request Jan 27, 2017

Closed

operations on Mat.empty #22

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Support the '0 dimension case' for bigarrays (#787)
Bigarray: support 0-dimension bigarrays (just a scalar value), with an Array0 module
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment