www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Formal Review of std.range.ndslice

reply Jack Stouffer <jack jackstouffer.com> writes:
This is the start of the two week formal review for the proposed 
std.range.ndslice. This new addition to the standard library 
would add the ability to create and manipulate multi-dimensional 
random access ranges in a way that will be very familiar to those 
of you who use numpy. This has the potential to give D a huge 
boost in popularity in numerical and scientific applications.

A quick run down for those that are not familiar with the 
process. After two weeks, the PR author (Ilya Yaroshenko) will 
have time to make proposed changes. Then, when the author feels 
it's ready, the PR will go to a vote. In the vote, everyone in 
the community has a say, but if one of the main contributors or 
maintainers has a very negative opinion (for example) that will 
carry more weight.

Github: https://github.com/D-Programming-Language/phobos/pull/3397
dub: http://code.dlang.org/packages/dip80-ndslice
docs: 
http://dtest.thecybershadow.net/results/bac6211c1d73b2cf62bc18c9844c8c82c92c21e1/5c6071ca953cf113febd8786b4b68916cbb2cdaf/

previous discussion: 
http://forum.dlang.org/thread/rilfmeaqkailgpxoziuo forum.dlang.org
Nov 16 2015
next sibling parent reply Brian Schott <briancschott gmail.com> writes:
On Monday, 16 November 2015 at 22:45:35 UTC, Jack Stouffer wrote:

 This is the start of the two week formal review for the 
 proposed std.range.ndslice. This new addition to the standard 
 library would add the ability to create and manipulate 
 multi-dimensional random access ranges in a way that will be 
 very familiar to those of you who use numpy. This has the 
 potential to give D a huge boost in popularity in numerical and 
 scientific applications.


One thing that stands out is the number of symbols in the Slice 
struct that have simple "///" doc comments. That is, they show up 
in the generated documentation without any explanation.
Nov 16 2015
parent Ilya Yaroshenko <ilyayaroshenko gmail.com> writes:
On Tuesday, 17 November 2015 at 00:10:42 UTC, Brian Schott wrote:

 On Monday, 16 November 2015 at 22:45:35 UTC, Jack Stouffer 
 wrote:

 This is the start of the two week formal review for the 
 proposed std.range.ndslice. This new addition to the standard 
 library would add the ability to create and manipulate 
 multi-dimensional random access ranges in a way that will be 
 very familiar to those of you who use numpy. This has the 
 potential to give D a huge boost in popularity in numerical 
 and scientific applications.


 One thing that stands out is the number of symbols in the Slice 
 struct that have simple "///" doc comments. That is, they show 
 up in the generated documentation without any explanation.


Thanks! Fixed
http://dtest.thecybershadow.net/artifact/website-7a646fdea76569e009844cdee5c93edab10980ca-2b25fcf8789289fb52e50e2e71914d1a/web/phobos-prerelease/std_experimental_range_ndslice.html

--Ilya
Nov 17 2015
prev sibling next sibling parent reply =?UTF-8?B?Tm9yZGzDtnc=?= <per.nordlow gmail.com> writes:
On Monday, 16 November 2015 at 22:45:35 UTC, Jack Stouffer wrote:

 This is the start of the two week formal review for the 
 proposed std.range.ndslice. This new addition to the standard


Why can't byElement



provide RandomAccess? It's documented to return an InputRange.
Nov 17 2015
parent reply =?UTF-8?B?Tm9yZGzDtnc=?= <per.nordlow gmail.com> writes:
On Tuesday, 17 November 2015 at 10:13:57 UTC, Nordlöw wrote:

 Why can't byElement



 provide RandomAccess? It's documented to return an InputRange.


AFAIK, if all the slice dimensions are know at compile-time it 
should. This is kind of similar to how std.range.chain() works.
Nov 17 2015
parent reply Ilya Yaroshenko <ilyayaroshenko gmail.com> writes:
On Tuesday, 17 November 2015 at 10:16:56 UTC, Nordlöw wrote:

 On Tuesday, 17 November 2015 at 10:13:57 UTC, Nordlöw wrote:

 Why can't byElement



 provide RandomAccess? It's documented to return an InputRange.


 AFAIK, if all the slice dimensions are know at compile-time it 
 should. This is kind of similar to how std.range.chain() works.


Thanks! Implemented: 
http://dtest.thecybershadow.net/artifact/website-7a646fdea76569e009844cdee5c93edab10980ca-4f243752aee5ecc098ce1c36eedd86a2/web/phobos-prerelease/std_experimental_range_ndslice.html#byElement

--Ilya
Nov 17 2015
next sibling parent =?UTF-8?B?Tm9yZGzDtnc=?= <per.nordlow gmail.com> writes:
On Tuesday, 17 November 2015 at 18:38:14 UTC, Ilya Yaroshenko 
wrote:

 Thanks! Implemented: 
 http://dtest.thecybershadow.net/artifact/website-7a646fdea76569e009844cdee5c93edab10980ca-4f243752aee5ecc098ce1c36eedd86a2/web/phobos-prerelease/std_experimental_range_ndslice.html#byElement

 --Ilya


Fantastic! Thx, for the quick response.
Nov 18 2015
prev sibling parent reply =?UTF-8?B?Tm9yZGzDtnc=?= <per.nordlow gmail.com> writes:
On Tuesday, 17 November 2015 at 18:38:14 UTC, Ilya Yaroshenko 
wrote:

 AFAIK, if all the slice dimensions are know at compile-time it 
 should. This is kind of similar to how std.range.chain() works.


 Thanks! Implemented: 
 http://dtest.thecybershadow.net/artifact/website-7a646fdea76569e009844cdee5c93edab10980ca-4f243752aee5ecc098ce1c36eedd86a2/web/phobos-prerelease/std_experimental_range_ndslice.html#byElement

 --Ilya


I don't see any other checks using `isRandomAccessRange`. Surely 
there must be other ranges/algorithms in ndslice that should 
propagate random access, right?
Nov 18 2015
parent Ilya Yaroshenko <ilyayaroshenko gmail.com> writes:
On Wednesday, 18 November 2015 at 10:18:37 UTC, Nordlöw wrote:

 On Tuesday, 17 November 2015 at 18:38:14 UTC, Ilya Yaroshenko 
 wrote:

 AFAIK, if all the slice dimensions are know at compile-time 
 it should. This is kind of similar to how std.range.chain() 
 works.


 Thanks! Implemented: 
 http://dtest.thecybershadow.net/artifact/website-7a646fdea76569e009844cdee5c93edab10980ca-4f243752aee5ecc098ce1c36eedd86a2/web/phobos-prerelease/std_experimental_range_ndslice.html#byElement

 --Ilya


 I don't see any other checks using `isRandomAccessRange`. 
 Surely there must be other ranges/algorithms in ndslice that 
 should propagate random access, right?


Only Slice and ByElements has random access. Special check was 
added for internal PtrShell, however it is commented with 
`version(none)` because it works only with git compiler (I have 
fixed std.internal.test.dummyrange ).

There was check using `isRandomAccessRange` after range 
primitives in Slice. I have added `hasLength` and `hasSlicing` 
too 
https://github.com/D-Programming-Language/phobos/commit/919009dce8903eca1ded119036ed5b2dd5bfcaa5

Updated docs: 
http://dtest.thecybershadow.net/artifact/website-7a646fdea76569e009844cdee5c93edab10980ca-3429854b54bfa5eb7060de221b45056f/web/phobos-prerelease/std_experimental_range_ndslice.html

-- Ilya
Nov 18 2015
prev sibling next sibling parent reply =?UTF-8?B?Tm9yZGzDtnc=?= <per.nordlow gmail.com> writes:
On Monday, 16 November 2015 at 22:45:35 UTC, Jack Stouffer wrote:

 docs: 
 http://dtest.thecybershadow.net/results/bac6211c1d73b2cf62bc18c9844c8c82c92c21e1/5c6071ca953cf113febd8786b4b68916cbb2cdaf/


There seems to be a mis-spelling of experimental as experemental:

Search for: std/experemental/range/ndslice.d
Nov 17 2015
parent =?UTF-8?B?Tm9yZGzDtnc=?= <per.nordlow gmail.com> writes:
On Tuesday, 17 November 2015 at 10:19:26 UTC, Nordlöw wrote:

 There seems to be a mis-spelling of experimental as 
 experemental:


Here: 
http://dtest.thecybershadow.net/artifact/website-7a646fdea76569e009844cdee5c93edab10980ca-2b25fcf8789289fb52e50e2e71914d1a/web/phobos-prerelease/std_experimental_range_ndslice.html
Nov 17 2015
prev sibling next sibling parent Jack Stouffer <jack jackstouffer.com> writes:
On Monday, 16 November 2015 at 22:45:35 UTC, Jack Stouffer wrote:

 docs: 
 http://dtest.thecybershadow.net/results/bac6211c1d73b2cf62bc18c9844c8c82c92c21e1/5c6071ca953cf113febd8786b4b68916cbb2cdaf/


I have already posted some things on Github, so If you will 
indulge me signing the praises of this addition for a moment:

Something dawned on me when I was scanning the numpy 
documentation, looking for anything obvious that was missed by 
this PR. What I noticed is that D really made the right bet when 
it came to putting a lot of range code in the standard library, 
because in order to make numpy useful at all, the authors had to 
recreate parts of the Python standard library in their C code so 
that it takes advantage of the type information that numpy arrays 
provide, where as all of std.algorithm and std.range can be used 
with this no problem. For example to create a zero initialized 
n-d array in numpy, you would have to use the numpy function 
np.zeros((10, 5)), but with this you can repeat(0, 50).sliced(10, 
5) with no performance penalty. In numpy to take the sum of a 
certain axis, you can use Python's len or sum like sum(array[:, 
0]) but it's slow so you have to remember the special case that 
you need to use array[:, 0].sum() instead to take advantage of 
numpy's types. With this it's just array[0..$, 0].sum with the 
same function you always use in the way you always use it. There 
are a ton of these special cases, and the consequence is it makes 
it very hard for non numpy code and numpy code to mix in a way 
that doesn't incur penalties or is a huge pain.

Also, this is a great showcase for how powerful D templates are 
as this solution is entirely a library solution where as numpy 
uses mountains of C code glued to Python. So, I think that this 
can be more powerful than numpy, as this fits naturally into the 
rest of your code base and can work efficiently with any range 
based code already written.
Nov 17 2015
prev sibling next sibling parent reply Robert burner Schadek <rburners gmail.com> writes:
I couldn't easily find how to make the module work with 
allocators. IMO combining this module with 
std.experiemtal.allocator should be possible. And if it is 
already possible, there should be tests for documentation and 
validation.
Nov 18 2015
next sibling parent John Colvin <john.loughran.colvin gmail.com> writes:
On Wednesday, 18 November 2015 at 08:52:11 UTC, Robert burner 
Schadek wrote:

 I couldn't easily find how to make the module work with 
 allocators. IMO combining this module with 
 std.experiemtal.allocator should be possible. And if it is 
 already possible, there should be tests for documentation and 
 validation.


https://github.com/D-Programming-Language/phobos/pull/3397#issuecomment-157652959
Nov 18 2015
prev sibling next sibling parent reply Ilya Yaroshenko <ilyayaroshenko gmail.com> writes:
On Wednesday, 18 November 2015 at 08:52:11 UTC, Robert burner 
Schadek wrote:

 I couldn't easily find how to make the module work with 
 allocators. IMO combining this module with 
 std.experiemtal.allocator should be possible. And if it is 
 already possible, there should be tests for documentation and 
 validation.


I have added makeSlice, however this function can be easily 
implemented by user: allocate array -> pass array to `sliced`.

updated docs 
http://dtest.thecybershadow.net/artifact/website-7a646fdea76569e009844cdee5c93edab10980ca-ce47155797f387348826317811c4af0c/web/phobos-prerelease/std_experimental_range_ndslice.html
--Ilya
Nov 18 2015
parent reply Jack Stouffer <jack jackstouffer.com> writes:
On Wednesday, 18 November 2015 at 10:26:13 UTC, Ilya Yaroshenko 
wrote:

 On Wednesday, 18 November 2015 at 08:52:11 UTC, Robert burner 
 Schadek wrote:

 I couldn't easily find how to make the module work with 
 allocators. IMO combining this module with 
 std.experiemtal.allocator should be possible. And if it is 
 already possible, there should be tests for documentation and 
 validation.


 I have added makeSlice, however this function can be easily 
 implemented by user: allocate array -> pass array to `sliced`.

 updated docs 
 http://dtest.thecybershadow.net/artifact/website-7a646fdea76569e009844cdee5c93edab10980ca-ce47155797f387348826317811c4af0c/web/phobos-prerelease/std_experimental_range_ndslice.html
 --Ilya


I spoke with Andrei over email and his opinion was that if this 
module makes any allocation that can't be avoided, then 
std.experimental.allocator should be integrated.
Nov 20 2015
parent reply Ilya <ilyayaroshenko gmail.com> writes:
On Friday, 20 November 2015 at 19:21:45 UTC, Jack Stouffer wrote:

 On Wednesday, 18 November 2015 at 10:26:13 UTC, Ilya Yaroshenko 
 wrote:

 On Wednesday, 18 November 2015 at 08:52:11 UTC, Robert burner 
 Schadek wrote:

 [...]


 I have added makeSlice, however this function can be easily 
 implemented by user: allocate array -> pass array to `sliced`.

 updated docs 
 http://dtest.thecybershadow.net/artifact/website-7a646fdea76569e009844cdee5c93edab10980ca-ce47155797f387348826317811c4af0c/web/phobos-prerelease/std_experimental_range_ndslice.html
 --Ilya


 I spoke with Andrei over email and his opinion was that if this 
 module makes any allocation that can't be avoided, then 
 std.experimental.allocator should be integrated.


Great! Added to PR's TODO list :-)
Nov 20 2015
parent reply Jack Stouffer <jack jackstouffer.com> writes:
On Friday, 20 November 2015 at 20:33:27 UTC, Ilya wrote:

 Great! Added to PR's TODO list :-)


This will be interesting because there is no defined idiomatic 
usage of std.allocator. I imagine std.range.ndslice will be the 
trend-setter for Phobos in this regard.
Nov 20 2015
next sibling parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 11/20/15 4:23 PM, Jack Stouffer wrote:

 On Friday, 20 November 2015 at 20:33:27 UTC, Ilya wrote:

 Great! Added to PR's TODO list :-)


 This will be interesting because there is no defined idiomatic usage of
 std.allocator. I imagine std.range.ndslice will be the trend-setter for
 Phobos in this regard.


There's the persistent list example that I posted on dpaste a couple 
weeks back. I do expect things to evolve a bit before stabilizing.

BTW thanks Jack for reaching out via email.


Andrei
Nov 20 2015
parent Ilya Yaroshenko <ilyayaroshenko gmail.com> writes:
On Friday, 20 November 2015 at 21:55:18 UTC, Andrei Alexandrescu 
wrote:

 On 11/20/15 4:23 PM, Jack Stouffer wrote:

 On Friday, 20 November 2015 at 20:33:27 UTC, Ilya wrote:

 Great! Added to PR's TODO list :-)


 This will be interesting because there is no defined idiomatic 
 usage of
 std.allocator. I imagine std.range.ndslice will be the 
 trend-setter for
 Phobos in this regard.


 There's the persistent list example that I posted on dpaste a 
 couple weeks back. I do expect things to evolve a bit before 
 stabilizing.

 BTW thanks Jack for reaching out via email.


 Andrei


After playing with memory allocation I became convinced that 
ndslice package should not have any kind of memory allocation 
(except opCast to array) at least we will add allocators support 
to std.array. The most important function is std.array.array.

Out of the box user can do `auto slice = new 
int[2*3*4+n].sliced(2, 3, 4); //n>=0`.
The link below contains convenience functions in examples for 
`sliced`
1. `createSlice` creates an GC allocated slice
2. `ndarray` creates GC allocated ndarray copy of a slice
3.  `makeSlice` make slice with allocators

They are only examples and not a part of the API.



--Ilya
Nov 21 2015
prev sibling parent Ilya <ilyayaroshenko gmail.com> writes:
On Friday, 20 November 2015 at 21:23:40 UTC, Jack Stouffer wrote:

 On Friday, 20 November 2015 at 20:33:27 UTC, Ilya wrote:

 Great! Added to PR's TODO list :-)


 This will be interesting because there is no defined idiomatic 
 usage of std.allocator. I imagine std.range.ndslice will be the 
 trend-setter for Phobos in this regard.


Slice is only a shell over a range/array. There is no reason to 
add any kind of counters or special stuff entities. All this 
idioms work out of the box because Slice holds an array. It is 
already works with std.container.array, and it will work with the 
future Array container based on allocators.

-- Ilya
Nov 20 2015
prev sibling parent reply Jack Stouffer <jack jackstouffer.com> writes:
On Wednesday, 18 November 2015 at 08:52:11 UTC, Robert burner 
Schadek wrote:

 I couldn't easily find how to make the module work with 
 allocators. IMO combining this module with 
 std.experiemtal.allocator should be possible. And if it is 
 already possible, there should be tests for documentation and 
 validation.


I know it's bad practice for regular Phobos code to import from 
std.experimental, but what's the protocol for code inside 
std.experimental importing other code in std.experimental? It 
would be nice to get Andrei's opinion on this.

Another problem with using std.experimental.allocator is it ties 
the time table of this going into stable with std.allocator. 
Worse case scenario, it could be a year before std.allocator is 
stable, where as std.range.ndslice is much simpler code and could 
be moved much faster.
Nov 18 2015
parent reply Ilya <ilyayaroshenko gmail.com> writes:
On Wednesday, 18 November 2015 at 18:40:40 UTC, Jack Stouffer 
wrote:

 On Wednesday, 18 November 2015 at 08:52:11 UTC, Robert burner 
 Schadek wrote:

 [...]


 I know it's bad practice for regular Phobos code to import from 
 std.experimental, but what's the protocol for code inside 
 std.experimental importing other code in std.experimental? It 
 would be nice to get Andrei's opinion on this.

 Another problem with using std.experimental.allocator is it 
 ties the time table of this going into stable with 
 std.allocator. Worse case scenario, it could be a year before 
 std.allocator is stable, where as std.range.ndslice is much 
 simpler code and could be moved much faster.


Possible solutions:
1. Mark `makeSlice` with red "Experimental"
2. Move `makeSlice` to unittest, so user could copy-past it.
Nov 18 2015
parent Jack Stouffer <jack jackstouffer.com> writes:
On Wednesday, 18 November 2015 at 19:25:04 UTC, Ilya wrote:

 Possible solutions:
 1. Mark `makeSlice` with red "Experimental"
 2. Move `makeSlice` to unittest, so user could copy-past it.


Another solution is to leave `makeSlice` in std.experimental when 
std.experimental.range.ndslice is moved to stable and move it to 
std.range when std.experimental.allocator is moved.

I think it would be best to get someone from the leadership's 
opinions on this before a decision is made.
Nov 18 2015
prev sibling next sibling parent reply Lucien Janvier <lulu meh.com> writes:
On Monday, 16 November 2015 at 22:45:35 UTC, Jack Stouffer wrote:

 This is the start of the two week formal review for the 
 proposed std.range.ndslice. This new addition to the standard 
 library would add the ability to create and manipulate 
 multi-dimensional random access ranges in a way that will be 
 very familiar to those of you who use numpy. This has the 
 potential to give D a huge boost in popularity in numerical and 
 scientific applications.

 [...]


I don't see any constraint related to dimenssion checking.
Nov 18 2015
parent Ilya Yaroshenko <ilyayaroshenko gmail.com> writes:
On Wednesday, 18 November 2015 at 20:10:00 UTC, Lucien Janvier 
wrote:

 On Monday, 16 November 2015 at 22:45:35 UTC, Jack Stouffer 
 wrote:

 This is the start of the two week formal review for the 
 proposed std.range.ndslice. This new addition to the standard 
 library would add the ability to create and manipulate 
 multi-dimensional random access ranges in a way that will be 
 very familiar to those of you who use numpy. This has the 
 potential to give D a huge boost in popularity in numerical 
 and scientific applications.

 [...]


 I don't see any constraint related to dimenssion checking.


I have add description to asserts 
https://github.com/9il/phobos/commit/f91d611ad66af9b6698142f92bcb8cc05747aff0

Is it constraints you asked?
Nov 18 2015
prev sibling next sibling parent Ilya Yaroshenko <ilyayaroshenko gmail.com> writes:
On Monday, 16 November 2015 at 22:45:35 UTC, Jack Stouffer wrote:

 This is the start of the two week formal review for the 
 proposed std.range.ndslice. This new addition to the standard 
 library would add the ability to create and manipulate 
 multi-dimensional random access ranges in a way that will be 
 very familiar to those of you who use numpy. This has the 
 potential to give D a huge boost in popularity in numerical and 
 scientific applications.

 A quick run down for those that are not familiar with the 
 process. After two weeks, the PR author (Ilya Yaroshenko) will 
 have time to make proposed changes. Then, when the author feels 
 it's ready, the PR will go to a vote. In the vote, everyone in 
 the community has a say, but if one of the main contributors or 
 maintainers has a very negative opinion (for example) that will 
 carry more weight.

 Github: 
 https://github.com/D-Programming-Language/phobos/pull/3397
 dub: http://code.dlang.org/packages/dip80-ndslice
 docs: 
 http://dtest.thecybershadow.net/results/bac6211c1d73b2cf62bc18c9844c8c82c92c21e1/5c6071ca953cf113febd8786b4b68916cbb2cdaf/

 previous discussion: 
 http://forum.dlang.org/thread/rilfmeaqkailgpxoziuo forum.dlang.org





1. Pay _unprecedented_ attention to functions to be
        a. inlined(!),
        b. ` nogc`,
        c. `nothrow`,
        d. `pure`.
     95% of functions will be marked with `pragma(inline, true)`. 
So, use
     _simple_ `assert`s with messages that can be computed at 
compile time.
     The goals are:
         1. to reduce executable size for _any_ compilation mode
         2. to reduce template bloat in object files
         3. to reduce compilation time
         4. to allow a user to write an extern C bindings for code 
based on `Slice`.

2. Do not import `std.format`, `std.string` and `std.conv` to 
format error
     messages.`"Use" ~ Concatenation.stringof ~ ", really ."` Why? 
Please,
     read [1] again.

3. Try to use already defined `mixin template`s for pretty error 
messaging.

4. Do not use `Exception`s/`enforce`s to check indexes and 
length. Exceptions are
     allowed only for algorithms where validation of an input data 
is
     significantly complex for user. `reshaped` is a good example 
where
     Exceptions are required. Put an example of Exception handing 
and workaround
     for function that can throw.

5. Do not use compile time flags for simple checks like 
transposition
     of a matrix. It is much better to have runtime matrix 
transposition.
     Furthermore, Slice provide runtime matrix transposition out 
of the box.

6. Do not use _Fortran_VS_C_ flags. They are about notation,
     but not about algorithm itself. To care about math world 
users add
     appropriate code example in the documentation. `transposed` / 
`everted`
     can be used for cash friendly code.

7. Do not use compile time power of D to produce dummy entities 
like
     `IdentityMatrix`.

8. Try to separate allocation and algorithm logic whenever 
possible.

9. Add CTFE unittests to new functions.

---

Update docs:
http://dtest.thecybershadow.net/artifact/website-7a646fdea76569e009844cdee5c93edab10980ca-87c7c7a1a6e59a71179301c54d012057/web/phobos-prerelease/std_experimental_range_ndslice.html

-- Ilya
Nov 20 2015
prev sibling next sibling parent reply Jack Stouffer <jack jackstouffer.com> writes:
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.
Nov 29 2015
parent reply Jack Stouffer <jack jackstouffer.com> writes:
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. Thank you to everyone who commented 
here or on the PR. Of course, even though the review is over, you 
can still make comments on the GitHub PR up until it's merged.

9il, please let me know if you want to start the voting right 
away or wait until your list is completed.
Dec 01 2015
next sibling parent Ilya Yaroshenko <ilyayaroshenko gmail.com> writes:
On Tuesday, 1 December 2015 at 16:03:51 UTC, 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. Thank you to everyone who 
 commented here or on the PR. Of course, even though the review 
 is over, you can still make comments on the GitHub PR up until 
 it's merged.

 9il, please let me know if you want to start the voting right 
 away or wait until your list is completed.


Thank you, Jack!

I plan to check English text and add 4D example for image 
processing and 5D example for computer vision first. They should 
help users to imagine use cases.

Plus looks like we need comparison between D.Slice and 
numpy.ndarray. Slice is more flexible, faster and generalised 
comparing with ndarray. In the same time Slice has not problems 
with extensions like numpy dose (as you have already noted) and 
it has tiny (in LOC) implementation. I hope that examples and the 
comparison would be moved later to DBlog (D needs blog!) with 
detailed explanation by another author (I can be reviewer).
Dec 01 2015
prev sibling parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
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
Dec 03 2015
next sibling parent Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 12/03/2015 10:07 AM, Andrei Alexandrescu wrote:

 * The code should use "private" appropriately.


And "package" etc. -- Andrei
Dec 03 2015
prev sibling parent reply Ilya Yaroshenko <ilyayaroshenko gmail.com> writes:
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
Dec 03 2015
parent Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 12/03/2015 11:50 AM, Ilya Yaroshenko wrote:

 No functions change data in `ndslice` package.


Then probably "-ed" is appropriate. -- Andrei
Dec 03 2015
prev sibling parent reply Stefan Frijters <sfrijters gmail.com> writes:
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
Dec 11 2015
parent reply Ilya <ilyayaroshenko gmail.com> writes:
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
Dec 11 2015
next sibling parent Ilya <ilyayaroshenko gmail.com> writes:
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
Dec 11 2015
prev sibling parent reply Stefan Frijters <sfrijters gmail.com> writes:
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?
Dec 13 2015
parent reply Ilya Yaroshenko <ilyayaroshenko gmail.com> writes:
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
Dec 13 2015
parent reply Stefan Frijters <sfrijters gmail.com> writes:
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).
Dec 13 2015
parent reply Ilya Yaroshenko <ilyayaroshenko gmail.com> writes:
On Sunday, 13 December 2015 at 21:24:36 UTC, Stefan Frijters 
wrote:

 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",
     },
 }


More reduced code:

import std.experimental.ndslice;

void main() {
	Slice!(3, double*) force = new double[60].sliced(3, 4, 5);

	// Correct code. Compiles!
	for(auto r = force.byElement; !r.empty; r.popFront)
	{
		size_t[3] p = r.index;
		double e = r.front;
	}

	// Wrong foreach params. dmd failed with exit code -11.
	//foreach(p, e; force)
	//{
	//}
}

dub.json
{
     "name": "dlbc",
     "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?


Yes
1. Please note in Bugzilla report that program code is incorrect 
(see example of correct above).
2. More reduced code can be used for report:

void main() {
	Slice!(3, double*) force = new double[60].sliced(3, 4, 5);
	// Wrong foreach params. dmd failed with exit code -11.
	foreach(p, e; force)
	{
	}
}


 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).


Probably you may need something like that:

auto ar = new double[60]; //remember ar
auto slice = ar.sliced(3, 4, 5);

However, if you have not changed slice's structure (except 
transposition), then

assert (ar == 
(&(slice.byElement.front()))[0..slice.elementsCount]);

Ilya
Dec 13 2015
parent Stefan Frijters <sfrijters gmail.com> writes:
On Sunday, 13 December 2015 at 22:32:43 UTC, Ilya Yaroshenko 
wrote:

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


 Yes
 1. Please note in Bugzilla report that program code is 
 incorrect (see example of correct above).
 2. More reduced code can be used for report:

 void main() {
 	Slice!(3, double*) force = new double[60].sliced(3, 4, 5);
 	// Wrong foreach params. dmd failed with exit code -11.
 	foreach(p, e; force)
 	{
 	}
 }


Reported as https://issues.dlang.org/show_bug.cgi?id=15441 .

Will look into my other non-bug problems when I can, have to run 
now...
Dec 14 2015