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

Bigarray: add Array[N].change_layout functions #1175

Merged

Conversation

Octachron
Copy link
Member

This pull requests proposes to add rank-specific change_layout functions to each Bigarray.Array[N] submodules.

Without these functions, changing the layout of a biggaray with a statistically known rank requires to go through a back-and-forth conversion to Bigarray.Genarray:

    let change_layout_[n] l a =
         array[n]_of_genarray
      @@ Genarray.change_layout l
      @@ genarray_of_array[n] a

Moreover, this back-and-forth conversion cannot exploit the fact that the change_layout function preserves rank, and thus it is not necessary to test the rank of the input biggarray in array[n]_of_genarray.

Since it is quite common to work with multidimensional arrays with statically known ranks, I think it makes sense for the bigarray library to directly define these functions rather than delegating their definition to library users.

This commit adds a rank-specific change_layout function to each
Array[N] submodules.

Without these functiond, changing the layout of a biggaray with a
statistically known rank requires to go through a back-and-forth
conversion to Genarray:

    let change_layout_[n] l a =
         array[n]_of_genarray
      @@ Genarray.change_layout l
      @@ genarray_of_array[n] a

This back-and-forth conversion cannot exploit the fact that the
`change_layout` function preserves rank, and thus it is not necessary
to test the rank of the input biggarray in `array[n]_of_genarray`.

Since it is quite common to work with a statically known rank, having
a version of `change_layout` available for each predefined rank seems
worthwhile.
Copy link
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine and I am planning to merge it. Two points:

  • You must add testsuite coverage for (at one least of) these functions.

  • You decided to export the external declaration in the .mli instead of a val, as the Genarray version does. But external are the cause of compatibility pains when changing things, and the advantage of not requiring linking the module is (1) I think gone now and (2) irrelevant for Bigarray anyway. Would using val declarations be better here?

@Octachron
Copy link
Member Author

I have added some testsuite coverage for the whole set of change_layout functions.
The new change_layout function are now exported as normal val rather than external since indeed the use of external was not warranted.

@gasche gasche merged commit 49b8c54 into ocaml:trunk Jun 6, 2017
stedolan pushed a commit to stedolan/ocaml that referenced this pull request Mar 21, 2023
Revert "Instance compilation units (ocaml#1113)"

This reverts commit 3bdccca.
EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024
* Document the OCaml Platform governance

This commit extends the OCaml.org governance to include the OCaml Platform. We
document the lifecycle and requirements for the OCaml Platform tools.

We also re-organise the structure of the governance to move the
OCaml.org-specific text in a single section.

Outside of minor text changes to mention the OCaml Platform, the following
sections remain untouched:

- Introduction
- Roles and responsibilities
- Processes

We also rename the document from "OCaml.org Governance" to "OCaml Governance" to
indicate that it is no longer specific to the OCaml.org project and sub-domains.

Following the goal of the document to describe the _existing_ governance and not
what we wish it was, the lifecycle and requirements for the Platform tools are
taken from how the OCaml Platform is currently governed. Both the lifecycle and
requirements have also been laid down in
[this talk](https://watch.ocaml.org/w/2KbfRNv2oLtkKXkbd5u9F1).

* Point to the infrastructure GitHub issue tracker instead of the mailing list
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