Skip to content

Conversation

9il
Copy link
Member

@9il 9il commented Apr 10, 2016

Allocation utilities was not implemented before because I needed a time to get real world experience with ndslice and allocators.

In addition, this PR fixes ugly unittests like

auto slice = (3*4*5*6).iota.array.sliced(3, 4, 5, 6);

Now they looks like

auto slice = iotaSlice(3, 4, 5, 6, 7).slice;

In addition, iotaSlice is faster than sliced iota, it has zero size and zero cost.

Mir

libmir/mir#38
libmir/mir#42
libmir/mir#44

New names

slice

  • SliceException
  • shape
  • slice (createSlice in mir PR)
  • makeSlice
  • ndarray
  • makeNdarray

selection

  • iotaSlice/IotaSlice

@9il 9il added the ndslice label Apr 10, 2016
@9il
Copy link
Member Author

9il commented Apr 10, 2016

ping @John-Colvin @wilzbach @JackStouffer

}

///
unittest
Copy link
Contributor

Choose a reason for hiding this comment

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

needs @nogc

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks!

@wilzbach
Copy link
Contributor

ping @John-Colvin @wilzbach @JackStouffer

I already tried my best to review the changes at mir - and I am still happy with the changes :)
Btw @9il I still think that diffing the changes between mir and std.experimental.ndslice will require quite some work in the future, especially when more changes come in at mir.

@9il
Copy link
Member Author

9il commented Apr 10, 2016

Btw @9il I still think that diffing the changes between mir and std.experimental.ndslice will require quite some work in the future, especially when more changes come in at mir.

I am thinking about automatic convetion tool ;)

@9il
Copy link
Member Author

9il commented Apr 10, 2016

@wilzbach what do you think about to rename createSlice to slice? We already have for example array and makeArray, ndarray and makeNdarray.

import std.range: iota, retro;
auto a = 20.iota.sliced(4, 5);
import std.experimental.ndslice.selection: iotaSlice;
auto a = iotaSlice(4, 5);

assert(a.dropExactly !(1, 0)(2, 3)[0, 0] == 17);
assert(a.dropExactly !(1, 0)(2, 3).shape == cast(size_t[2])[1, 3]);
Copy link
Contributor

Choose a reason for hiding this comment

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

why is there so much whitespace here?

Copy link
Member Author

Choose a reason for hiding this comment

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

allgned with lines below

@JackStouffer
Copy link
Contributor

Besides testing, what's the rationale for iotaSlice?

@wilzbach
Copy link
Contributor

@wilzbach what do you think about to rename createSlice to slice? We already have for example array and makeArray, ndarray and makeNdarray.

I was actually thinking about this too. Initially I wanted to suggest that we use the same method name for both the gc and allocation version, but prefixing it with make does make sense. So +1 for slice and a common naming convention.

@9il
Copy link
Member Author

9il commented Apr 10, 2016

Besides testing, what's the rationale for iotaSlice?

This is good building block for abstract algorithms along with indexSlice.

{
size_t[2] shape = (int[][]).init.shape;
assert(shape[0] == 0);
assert(shape[1] == 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

could you add a test for a higher-dimensional array?
We only test 2d in all tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

2D is OK for recursion algorithms. btw, amount of unittests in ndslice already largest in Phobos except std.datetime.

Copy link
Contributor

Choose a reason for hiding this comment

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

2D is OK for recursion algorithms

I was just worried that we create a bias with the tests by only testing 2d.

amount of unittests in ndslice already largest in Phobos except std.datetime

How did you measure this?

Last time I checked there were some parts in Phobos with 100% coverage.

Copy link
Member Author

Choose a reason for hiding this comment

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

Last time I checked there were some parts in Phobos with 100% coverage.

I mean amount of LOC

Copy link
Member Author

Choose a reason for hiding this comment

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

How did you measure this?

I just know probos pretty well)

@wilzbach
Copy link
Contributor

needs rebase :(

@PetarKirov
Copy link
Member

@9il I have an alternative idea about the allocation API:
Merge ndarray and makeNdarray into a single function and use alias template parameter for the allocator:

// Before:
auto ndarray(size_t N, Range)
    (auto ref Slice!(N, Range) slice);
auto makeNdarray(T, Allocator, size_t N, Range)
    (auto ref Allocator alloc,  Slice!(N, Range) slice);

// After:
auto makeNdarray(alias allocator = GCAllocator, size_t N, Range)
    (auto ref Slice!(N, Range) slice)
{
    alias alloc = allocatorInstanceOf!(allocator);
    alias T = ElementType!Range;

    // same as before...
}

auto m1 = slice.makeNdarray;
auto m2 = slice.makeNdarray!MmapAllocator;
auto m3 = slice.makeNdarray!theAllocator;
auto m4 = slice.makeNdarray!(Mallocator.instance);

(Ditto for makeSlice).

Full example: http://dpaste.dzfl.pl/c3894699a269

@wilzbach
Copy link
Contributor

Merge ndarray and makeNdarray into a single function and use alias template parameter for the allocator:

We tried this before, but unfortunately GCAllocator isn't pure :/

@9il
Copy link
Member Author

9il commented Apr 12, 2016

Merge ndarray and makeNdarray into a single function and use alias template parameter for the allocator:
We tried this before, but unfortunately GCAllocator isn't pure :/

In addition, current API is similar to array and makeArray+dispose. ndarray is good name, btw)

@PetarKirov
Copy link
Member

@wilzbach

We tried this before, but unfortunately GCAllocator isn't pure :/

Then we should fix GCAllocator ;) I find std.experimental.allocator surprisingly lacking in terms of attributes. I did add some, but I didn't look at gc_allocator and mmap_allocator because I wasn't needing them at the time. pure is going to be the hard one because Allocator.instance is mutable and shared. Also, it is debatable whether this attribute should be used for anything but GCAllocator.allocate.
OTOH, pure-ity is easy for stateful allocators - example.

@9il

In addition, current API is similar to array and makeArray+dispose. ndarray is good name, btw)

OK, but what do you think about only changing makeNdarray to the the API I proposed, which IMO is easier / more flexible to use:

auto makeNdarray(alias allocator = GCAllocator, size_t N, Range)
    (auto ref Slice!(N, Range) slice);

auto m1 = slice.makeNdarray;
auto m2 = slice.makeNdarray!MmapAllocator;
auto m3 = slice.makeNdarray!theAllocator;
auto m4 = slice.makeNdarray!(Mallocator.instance);

@9il
Copy link
Member Author

9il commented Apr 12, 2016

OK, but what do you think about only changing makeNdarray to the the API I proposed, which IMO is easier / more flexible to use:

If @andralex change it for makeArray too.

@wilzbach
Copy link
Contributor

Then we should fix GCAllocator ;) [...] pure is going to be the hard one because Allocator.instance is mutable and shared. On the other hand, it is debatable whether this attribute should be used for anything but GCAllocator.allocate.

I submitted #4190 which hopefully explains the problem that requires us to provide two functions for everything that uses the allocation API.

@9il
Copy link
Member Author

9il commented Apr 16, 2016

@DmitryOlshansky Could you please merge this. It is blocker for other fixes ( #4203 ) and Mir.

@DmitryOlshansky
Copy link
Member

Auto-merge toggled on

@9il
Copy link
Member Author

9il commented Apr 16, 2016

Thanks!

@DmitryOlshansky DmitryOlshansky merged commit a0edfec into dlang:master Apr 16, 2016
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.

5 participants