On 12/01/2015 11:03 AM, Jack Stouffer wrote:
> On Sunday, 29 November 2015 at 20:53:43 UTC, Jack Stouffer wrote:
>> On Monday, 16 November 2015 at 22:45:35 UTC, Jack Stouffer wrote:
>>> This is the start of the two week formal review
>>
>> Friendly reminder that the review ends tomorrow.
>
> The two week review is over.

Thanks to all who participated in this. This is not a formal review, but I have a few comments (some of which echo what others said). They are not blocking, i.e. they shouldn't be addressed before voting.

DOCUMENTATION

* A native English speaker should make a careful pass through the documentation, there are a few simple things that are likely to escape systematically to a non-native. E.g. "which are represented with the Slice." -> "which are represented with the Slice type."

* There should be a synopsis at the very beginning of the module with a short complete example illustrating what the package can be used for.

* The use of "bifacial" for template and non-template is new?

* sliced() doesn't explain what happens if the input size is not a multiple of the sizes.

* Do createSlice() and ndarray() work with allocators?

* Not a fan of non-imperative function names for imperative functions, i.e. transposed(), swapped(), everted() etc. stand out like so many sore thumbs. For example the description of reversed() says "ReverseS direction of the iteration" so I assume it's just doing it imperatively. In Phobos we generally use action verbs for imperative actions and "-ed" for lazy behavior. Would be nice to continue that.

* Why does byElement return just an input range (and not a more powerful one)?

* Typo: std/experemental/range/ndslice.d

* Documentation is incomplete at the bottom of std.experimental.range.ndslice, i.e. there are a bunch of names without descriptions.

* Why is typeof((new int [1000].sliced(5, 6, 7)) Slice!(3, int*) and not Slice!(3, int[])? That seems to suggest there's little @safety in the interface.

IMPLEMENTATION

* According to git grep, in phobos there are 8139 occurrences of 'if (' and 2451 occurrences of 'if('. We should avoid such jitters in new code. Please change if/while/for/foreach/etc to include a space before the paren (actually some do have the space, others don't). There are other style violations as well, e.g. no space around operators (but only sometimes, inconsistently) etc. For existing code it's a slow process but this is new code and this is the right time to make it flush with the overall phobos style.

* The code should use "private" appropriately.

* Code should use local imports wherever possible.

* There's some commented-out code there, shouldn't.

* Duplication easy to eliminate between isPermutation and isPartialPermutation, just have the first call the second and then do the extra work.

* I think we have something good for isReference/hasReference in std.traits.

* Unittests should use @safe wherever possible, which may prompt changes in the interface and implementation.

Very nice work. Thanks!


Andrei

On 12/03/2015 10:07 AM, Andrei Alexandrescu wrote:
> * The code should use "private" appropriately.

And "package" etc. -- Andrei

On Thursday, 3 December 2015 at 15:07:55 UTC, Andrei Alexandrescu wrote:
> On 12/01/2015 11:03 AM, Jack Stouffer wrote:
>> On Sunday, 29 November 2015 at 20:53:43 UTC, Jack Stouffer wrote:
>>> On Monday, 16 November 2015 at 22:45:35 UTC, Jack Stouffer wrote:
>>>> This is the start of the two week formal review
>>>
>>> Friendly reminder that the review ends tomorrow.
>>
>> The two week review is over.
>
> Thanks to all who participated in this. This is not a formal review, but I have a few comments (some of which echo what others said). They are not blocking, i.e. they shouldn't be addressed before voting.
>
> DOCUMENTATION
>
> * A native English speaker should make a careful pass through the documentation, there are a few simple things that are likely to escape systematically to a non-native. E.g. "which are represented with the Slice." -> "which are represented with the Slice type."

I will create review with professional translator at least.

> * There should be a synopsis at the very beginning of the module with a short complete example illustrating what the package can be used for.

Plans to add one example for image processing and one for computer vision.

> * The use of "bifacial" for template and non-template is new?

Yes

> * sliced() doesn't explain what happens if the input size is not a multiple of the sizes.

will fix

> * Do createSlice() and ndarray() work with allocators?

Looks like you have read old docs. I have added a lot features and split module into package. LOC growed from 1.5K to 4.5K

Quick brief of changes can be found in https://github.com/D-Programming-Language/phobos/pull/3397 .

The latest docs (right now): http://dtest.thecybershadow.net/artifact/website-8937f229ab6d7bffa1c5996673347d0071563ef1-44ccae0e926aef9268d7289ec985a497/web/phobos-prerelease/std_experimental_ndslice.html

ALLOCATORS:

1. There is no any kind off allocation (string concatenations in asserts still needs to be checked) in updated std.experimental.ndslice package except the `ReshapeException` in the `reshape` function.

2. There is tiny example for `sliced` with `makeSlice` function, which user can copy-past to work with allocators.

3. `ndarray` is an example too. To make it work with allocators we need std.array.array to work with allocators first.

> * Not a fan of non-imperative function names for imperative functions, i.e. transposed(), swapped(), everted() etc. stand out like so many sore thumbs. For example the description of reversed() says "ReverseS direction of the iteration" so I assume it's just doing it imperatively. In Phobos we generally use action verbs for imperative actions and "-ed" for lazy behavior. Would be nice to continue that.

OK, `*ed` will be removed.

Simplest 1D Example for reverse:

data: 0 1 2 3 4 5
      ^
      ptr, stride = 1

after reverse:
data: 0 1 2 3 4 5
                ^
                ptr, stride = -1

No functions change data in `ndslice` package.

In updated docs there is two kind of functions:

1. std.experimental.ndslice.iteration contains functions like `transposed` and other `*ed` stuff (`*ed`  will be removed).
   1. This functions do _not_ change data
   2. Have the _same_ return type

2. std.experimental.ndslice.selection contains functions like `reshape, `blocks` and `byElement`.
   1. This functions do _not_ change data
   2. Have _another_ return type

> * Why does byElement return just an input range (and not a more powerful one)?

Fixed to Random Access Range with slicing.
Please use the link above to access updated docs. And PR to access the latest docs via CyberShadow doc engine.

> * Typo: std/experemental/range/ndslice.d
>
> * Documentation is incomplete at the bottom of std.experimental.range.ndslice, i.e. there are a bunch of names without descriptions.

Partially fixed, still waiting for my translator

> * Why is typeof((new int [1000].sliced(5, 6, 7)) Slice!(3, int*) and not Slice!(3, int[])? That seems to suggest there's little @safety in the interface.

`ReplaceArrayWithPointer` flag was added. By default it is `true`. It can be used for CTFE-able code. Performance difference between pointer and array is very signifiant:

Binary structure with pointer:
  lengths
  strides
  ptr

Binary structure with array:
  lengths
  strides
  array (pointer + length)
  shift (shift to the front element in a slice)

It is impossible to have flexible engine for arrays/ranges without `shift`. Pointers have not such constraints.

> IMPLEMENTATION
>
[...]
> Very nice work. Thanks!
>
>
> Andrei

Thank you for review!

Ilya

On 12/03/2015 11:50 AM, Ilya Yaroshenko wrote:
> No functions change data in `ndslice` package.

Then probably "-ed" is appropriate. -- Andrei

Today I've made an abortive attempt at replacing my code's [1] dependence on unstd.multidimarray [2] with ndslice.
I'm guessing it's just me being stupid, but could anyone supply with some hints on how to do the conversion with a minimum of fuss?

Basically I have an N-dimensional array (N is known at compile time) of type T wrapped in a struct to carry some additional information.
I then address it using size_t[N] fixed-size arrays, and loop over it a lot with foreach, which also uses size_t[N] as index.

So it looks something like this:

struct Field(T, uint N) {

  alias arr this;

  MultidimArray!(T, N) arr; // is there any way to supply the correct type here with ndslice? I cannot use auto, right?

  this (in size_t[N] lengths) {
    arr = multidimArray!T(lengths);
  }
}
and then things like

foreach(immutable p, ref pop; someField) {
  pop = foo(someOtherField[bar(p)]);
  ...
}

where p is of type size_t[N].

I tried using ndarray in conjunction with the std.experimental.allocator, but I don't particularly care about memory management;
these are large arrays that are allocated once and kept around for the duration of the program.

Any help would be appreciated.

[1] https://github.com/SFrijters/DLBC
[2] https://bitbucket.org/SFrijters/unstandard

On Friday, 11 December 2015 at 19:31:14 UTC, Stefan Frijters wrote:
> Today I've made an abortive attempt at replacing my code's [1] dependence on unstd.multidimarray [2] with ndslice.
> I'm guessing it's just me being stupid, but could anyone supply with some hints on how to do the conversion with a minimum of fuss?
>
> Basically I have an N-dimensional array (N is known at compile time) of type T wrapped in a struct to carry some additional information.
> I then address it using size_t[N] fixed-size arrays, and loop over it a lot with foreach, which also uses size_t[N] as index.
>
> So it looks something like this:
>
> struct Field(T, uint N) {
>
>   alias arr this;
>
>   MultidimArray!(T, N) arr; // is there any way to supply the correct type here with ndslice? I cannot use auto, right?

    Slice!(N, T*) arr;

>
>   this (in size_t[N] lengths) {
>     arr = multidimArray!T(lengths);

      // compute length
      // more flexible construtors would be added after
      // allocatrs support for ndslice
      size_t len = 1;
      foreach(l; lengths)
         len *= l;

      arr = new T[len].sliced(lengths);

>   }
> }
> and then things like
>
> foreach(immutable p, ref pop; someField) {
>   pop = foo(someOtherField[bar(p)]);
>   ...
> }

   std.experimental.ndslice.selection: indexSlice, byElement;

   foreach(p; someField.shape.indexSlice.byElement) {
   	  someField[p] = foo(someOtherField[bar(p)]);
          ...
   }

> where p is of type size_t[N].
>
> I tried using ndarray in conjunction with the std.experimental.allocator, but I don't particularly care about memory management;
> these are large arrays that are allocated once and kept around for the duration of the program.
>
> Any help would be appreciated.
>
> [1] https://github.com/SFrijters/DLBC
> [2] https://bitbucket.org/SFrijters/unstandard

See also updated docs: http://dtest.thecybershadow.net/artifact/website-13cbdcf17d84fc31328c3f517a56bea783c418d6-d9c63e815273f0906309088334e7dfb1/web/phobos-prerelease/std_experimental_ndslice.html

Ilya

On Friday, 11 December 2015 at 22:56:15 UTC, Ilya wrote:
> On Friday, 11 December 2015 at 19:31:14 UTC, Stefan Frijters wrote:
>> Today I've made an abortive attempt at replacing my code's [1] dependence on unstd.multidimarray [2] with ndslice.
>> I'm guessing it's just me being stupid, but could anyone supply with some hints on how to do the conversion with a minimum of fuss?
>>
>> Basically I have an N-dimensional array (N is known at compile time) of type T wrapped in a struct to carry some additional information.
>> I then address it using size_t[N] fixed-size arrays, and loop over it a lot with foreach, which also uses size_t[N] as index.
>>
>> So it looks something like this:
>>
>> struct Field(T, uint N) {
>>
>>   alias arr this;
>>
>>   MultidimArray!(T, N) arr; // is there any way to supply the correct type here with ndslice? I cannot use auto, right?
>
>     Slice!(N, T*) arr;
>
>>
>>   this (in size_t[N] lengths) {
>>     arr = multidimArray!T(lengths);
>
>       // compute length
>       // more flexible construtors would be added after
>       // allocatrs support for ndslice
>       size_t len = 1;
>       foreach(l; lengths)
>          len *= l;
>
>       arr = new T[len].sliced(lengths);
>
>>   }
>> }
>> and then things like
>>
>> foreach(immutable p, ref pop; someField) {
>>   pop = foo(someOtherField[bar(p)]);
>>   ...
>> }
>
>    std.experimental.ndslice.selection: indexSlice, byElement;
>
>    foreach(p; someField.shape.indexSlice.byElement) {
>    	  someField[p] = foo(someOtherField[bar(p)]);
>           ...
>    }

faster version:

std.experimental.ndslice.selection: byElement;

for(auto r = someField.arr.byEleemnt; r.popFront) {
	r.front = foo(someOtherField[bar(r.index)]);
	...
}
>> where p is of type size_t[N].

Ilya

On Friday, 11 December 2015 at 22:56:15 UTC, Ilya wrote:
> On Friday, 11 December 2015 at 19:31:14 UTC, Stefan Frijters wrote:
>> [...]
>
>     Slice!(N, T*) arr;
>
>>     [...]
>
>       // compute length
>       // more flexible construtors would be added after
>       // allocatrs support for ndslice
>       size_t len = 1;
>       foreach(l; lengths)
>          len *= l;
>
>       arr = new T[len].sliced(lengths);
>
>> [...]
>
>    std.experimental.ndslice.selection: indexSlice, byElement;
>
>    foreach(p; someField.shape.indexSlice.byElement) {
>    	  someField[p] = foo(someOtherField[bar(p)]);
>           ...
>    }
>
>> [...]
>
> See also updated docs: http://dtest.thecybershadow.net/artifact/website-13cbdcf17d84fc31328c3f517a56bea783c418d6-d9c63e815273f0906309088334e7dfb1/web/phobos-prerelease/std_experimental_ndslice.html
>
> Ilya

Thank you for your help. I'm trying to convert my code again at the moment, but ran into a new problem: I need to pass a pointer to the data into a C function. It seems that the .ptr property is not available, and using & caused dmd to segfault (currently running a Dustmite reduction for that). Is there any clean way to get a pointer to the underlying data?

On Sunday, 13 December 2015 at 15:25:11 UTC, Stefan Frijters wrote:
> On Friday, 11 December 2015 at 22:56:15 UTC, Ilya wrote:
>> On Friday, 11 December 2015 at 19:31:14 UTC, Stefan Frijters wrote:
>>> [...]
>>
>>     Slice!(N, T*) arr;
>>
>>>     [...]
>>
>>       // compute length
>>       // more flexible construtors would be added after
>>       // allocatrs support for ndslice
>>       size_t len = 1;
>>       foreach(l; lengths)
>>          len *= l;
>>
>>       arr = new T[len].sliced(lengths);
>>
>>> [...]
>>
>>    std.experimental.ndslice.selection: indexSlice, byElement;
>>
>>    foreach(p; someField.shape.indexSlice.byElement) {
>>    	  someField[p] = foo(someOtherField[bar(p)]);
>>           ...
>>    }
>>
>>> [...]
>>
>> See also updated docs: http://dtest.thecybershadow.net/artifact/website-13cbdcf17d84fc31328c3f517a56bea783c418d6-d9c63e815273f0906309088334e7dfb1/web/phobos-prerelease/std_experimental_ndslice.html
>>
>> Ilya
>
> Thank you for your help. I'm trying to convert my code again at the moment, but ran into a new problem: I need to pass a pointer to the data into a C function. It seems that the .ptr property is not available, and using & caused dmd to segfault (currently running a Dustmite reduction for that). Is there any clean way to get a pointer to the underlying data?

Could you please post reduced code example that caused dmd to segfault?

2D way: &slice[0, 0]   or   &(slice.front.front());

ND way: &(slice.byElement.front())

Note: Comparing with unstandard  there is no guarantee that the first element in a ndarray is the first element in memory. `reversed` and `allReversed` should not be used to preserve strides positive.

The latests docs (with fixed English): http://dtest.thecybershadow.net/artifact/website-13cbdcf17d84fc31328c3f517a56bea783c418d6-dd2292a424959b594956eeeba64d391f/web/phobos-prerelease/std_experimental_ndslice.html

Voting For std.experimental.ndslice http://forum.dlang.org/post/ztaudqxmjrgnilsioyqs@forum.dlang.org
Please Vote!

Ilya

On Sunday, 13 December 2015 at 15:59:19 UTC, Ilya Yaroshenko wrote:
> Could you please post reduced code example that caused dmd to segfault?

Took dustmite about 6 hours to reduce, and then I went at it manually for a bit, so this is the smallest I could get it:

import std.experimental.ndslice;

int main() {
  Field force;
  foreach(p, e; force) e;
}

struct Field {
  alias arr this;
  Slice!(3, double*) arr;
}

Compiled with dmd 2.069.1 via dub build:

{
    "name": "dlbc",
    "sourcePaths": ["src"],
    "dependencies": {
        "dip80-ndslice": "~>0.8.3",
    },
}

dip80-ndslice 0.8.3: target for configuration "library" is up to date.
dlbc ~master: building configuration "application"...
Segmentation fault
dmd failed with exit code 139.

Since it's a segfault in the compiler, should I put it on Bugzilla too?

> 2D way: &slice[0, 0]   or   &(slice.front.front());
>
> ND way: &(slice.byElement.front())
>
> Note: Comparing with unstandard  there is no guarantee that the first element in a ndarray is the first element in memory. `reversed` and `allReversed` should not be used to preserve strides positive.

Hm, I assumed the underlying array would be a single block of data and then a bunch of pointers would be used to keep track of any slices. I'll try to figure out how to give the data to C then (for MPI and HDF5, to be exact).