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

Support bigarray with 0 dimension as scalar #7339

Closed
vicuna opened this issue Aug 30, 2016 · 4 comments

Comments

Projects
None yet
2 participants
@vicuna
Copy link

commented Aug 30, 2016

Original bug ID: 7339
Reporter: laurent.mazare
Assigned to: @gasche
Status: resolved (set by @gasche on 2016-09-01T17:18:39Z)
Resolution: fixed
Priority: normal
Severity: feature
Fixed in version: 4.05.0 +dev/beta1/beta2/beta3/rc1
Category: otherlibs
Monitored by: @yallop

Bug description

I'm wondering if there is some specific reason for not supporting the '0 dimension' case for bigarrays. This case is handled in some other numerical libraries, e.g. numpy or tensorflow, with the natural semantics of being a scalar value.

I tried relaxing the exceptions that currently prevent this case in the branch below + added some tests and it seems to work fine with a quite simple change:
trunk...LaurentMazare:trunk

Note that this doesn't include any support for the bigarray.{} syntax, not sure how worthy this is.

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.

Would this sound like a reasonable change ?

@vicuna

This comment has been minimized.

Copy link
Author

commented Aug 30, 2016

Comment author: @gasche

It does sound like a reasonable change to me, you should consider submitting a github pull request.

(You could squash the commits in your branch, and you should add a Changes entry in the "Next version" section.)

@vicuna

This comment has been minimized.

Copy link
Author

commented Aug 30, 2016

Comment author: laurent.mazare

Ok thanks I've added the Changes entry and submitted a PR.

@vicuna

This comment has been minimized.

Copy link
Author

commented Sep 1, 2016

Comment author: laurent.mazare

For reference, the PR is #787

@vicuna

This comment has been minimized.

Copy link
Author

commented Sep 1, 2016

Comment author: @gasche

The proposed implementation is now merged in trunk (will not be part of the feature-frozen 4.04.0, but of the next release after that, probably 4.05). Thanks!

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.