Add byDim function #79

jmh530 · 2017-08-01T23:13:47Z

Adds convenience functions byDim and apply. byDim provides iteration of a slice by dimension, while apply combines byDim and map to apply a function by dimension.

Functionality discussed in issue #58.

codecov-io · 2017-08-02T02:35:34Z

Codecov Report

Merging #79 into master will increase coverage by 0.24%.
The diff coverage is 100%.

@@ Coverage Diff @@ ## master #79 +/- ## ========================================== + Coverage 96.32% 96.56% +0.24%  ========================================== Files 29 29 Lines 4761 5096 +335 ========================================== + Hits 4586 4921 +335  Misses 175 175

Impacted Files	Coverage Δ
source/mir/ndslice/package.d	`92.25% <ø> (ø)`	⬆️
source/mir/ndslice/topology.d	`99.26% <100%> (+0.32%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6cf627a...88409fc. Read the comment docs.

9il · 2017-08-02T12:12:46Z

source/mir/ndslice/topology.d

+
+ auto adjTransposed(SliceKind kind, size_t[] packs, Iterator)(Slice!(kind, packs, Iterator) slice)
+ {
+ static assert (packs != [1], "1-dimensional slice not accepted");


this can be removed and generalised for vectors

@9il I had only included that initially because transposed does not work properly on vectors, i.e.

auto slice = iota(3); auto x = slice.transposed!1; //does not work

but with the adjustment for packs, then one can only pass dim=0 for a 1-dimensional vector and there is no error.

9il · 2017-08-02T12:13:51Z

source/mir/ndslice/topology.d

+{
+ import mir.ndslice : Slice, SliceKind;
+
+ auto adjTransposed(SliceKind kind, size_t[] packs, Iterator)(Slice!(kind, packs, Iterator) slice)


please add ///

@9il Do I also need to add the /// before byDim?

9il · 2017-08-02T12:14:31Z

source/mir/ndslice/topology.d

+ import mir.ndslice : Slice, SliceKind;
+
+ auto adjTransposed(SliceKind kind, size_t[] packs, Iterator)(Slice!(kind, packs, Iterator) slice)
+ {


if (dim < packs[0])

9il · 2017-08-02T12:16:18Z

source/mir/ndslice/topology.d

+ {
+ return slice;
+ }
+ else


... else static if (dim + 1 < packs[0] || packs.length > 1) { import mir.ndslice : canonical, transposed; return slice.canonical.transposed!dim; } ...

please cover this in unittest

9il · 2017-08-02T12:17:22Z

source/mir/ndslice/topology.d

+
+ auto slice = iota(3, 4);
+
+ static immutable shape34 = [3, 4];


Please change to fixed size arrays (like size_t[2] = shape34 = [3, 4]; ) for all unittests

9il · 2017-08-02T12:21:37Z

source/mir/ndslice/package.d

 $(TD
- $(SUBREF topology, as)
+ $(SUBREF topology, apply)
+$(SUBREF topology, as)


tabs -> spaces

9il · 2017-08-02T12:25:39Z

source/mir/ndslice/topology.d

+ auto adjTransposed(SliceKind kind, size_t[] packs, Iterator)(Slice!(kind, packs, Iterator) slice)
+ {
+ static assert (packs != [1], "1-dimensional slice not accepted");
+


Please remove trailing tabs and spaces

in all edited files

9il · 2017-08-02T12:30:13Z

source/mir/ndslice/topology.d

+$(LREF ipack),
+$(LREF transposed).
+/
+template byDim(size_t dim = 0)


Please generalize it for multiple dimensions like it was done for transposed:

template transposed(Dimensions...) if (Dimensions.length) { static if (!allSatisfy!(isSize_t, Dimensions)) alias transposed = .transposed!(staticMap!(toSize_t, Dimensions)); else ....

9il · 2017-08-02T12:34:49Z

source/mir/ndslice/topology.d

 $(T2 ipack , Returns slice of slices.)
 $(T2 unpack , Merges all dimension packs.)
 $(T2 evertPack, Reverses dimension packs.)
+$(T2 byDim , Convenience function to return a slice that can be iterated by dimension.)


Please also add a note what actually this function do: transposes dimensions on top and packs them.

9il · 2017-08-02T12:38:46Z

source/mir/ndslice/topology.d

+ $(LREF map),
+ $(HTTP https://stat.ethz.ch/R-manual/R-devel/library/base/html/apply.html, apply)
+/
+template apply(alias f, size_t dim = 0)


I do not think we need this function. slice.byDim!dim.map!f is well enough. Also issue with apply name: apply is a verb in infinitive form, which is good for actual computations but not for lazy data type construction.

Fair point on the naming, R's apply would really be more like slice.byDim!dim.each!f

9il · 2017-08-02T12:39:25Z

ping @wilzbach : what do you think?

9il · 2017-08-02T14:49:20Z

source/mir/ndslice/topology.d

+ import mir.ndslice.slice : Slice, SliceKind;
+
+ ///
+ auto adjTransposed(SliceKind kind, size_t[] packs, Iterator)(Slice!(kind, packs, Iterator) slice)


Please add @fastmath flag for all inner functions

jmh530 · 2017-08-03T02:25:40Z

@9il I think I have made all of the requested changes.

Let me know if additional changes are required, or if you want me to rebase the three commits into one final one before merging (I already compressed a bunch into the two additional ones).

9il · 2017-08-03T03:02:54Z

source/mir/ndslice/topology.d

+ alias byDim = .byDim!(staticMap!(toSize_t, Dimensions));
+ else
+ ///
+ auto byDim(SliceKind kind, size_t[] packs, Iterator)


@fastmath

9il · 2017-08-03T03:03:48Z

source/mir/ndslice/topology.d

+}
+
+///
+@fastmath private auto adjTransposed(SliceKind kind, size_t[] packs, Iterator)


no need @fastmath here because it is global for the module

9il · 2017-08-03T03:04:06Z

source/mir/ndslice/topology.d

 }
+
+/// Transposed adjusted to ignore dim=0 and include universal
+@fastmath private template adjTransposed(Dimensions...)


no need @fastmath here because it is global for the module

9il · 2017-08-03T03:04:42Z

source/mir/ndslice/topology.d

+ alias adjTransposed = .adjTransposed!(staticMap!(toSize_t, Dimensions));
+ else
+ ///
+ auto adjTransposed(SliceKind kind, size_t[] packs, Iterator)(Slice!(kind, packs, Iterator) slice)


@fastmath is required here because global fastmath does not affect internal template content

9il · 2017-08-03T03:12:05Z

source/mir/ndslice/topology.d

+ {
+ return slice;
+ }
+ else


Please add specialization for canonical case. Canonical case is valid when the last (last of unpacked slice) dimension of contiguous slice is not moved upper. The code example for single dimension is below. Please generalize it for multiple dimensions.

... else static if (dim + 1 < packs[0] || packs.length > 1) { import mir.ndslice : canonical, transposed; return slice.canonical.transposed!dim; } ...

please cover this in unittest (contiguous -> canonical) . Existing unittests should be enough, just extend them

9il · 2017-08-03T03:14:13Z

source/mir/ndslice/topology.d

+
+ mixin DimensionsCountCTError;
+
+ static if (Dimensions.length == 1 && Dimensions[0] == 0)


static if ([Dimensions] == [Iota!(Dimensions.length)])
or
static if (Dimensions == Iota!(Dimensions.length)) but not sure if the last variant works

jmh530 · 2017-08-03T12:28:31Z

@9il I have fixed some of the issues...I definitely didn't understand your point on the canonical the first time you mentioned it. This second time I've at least added the code and adjusted it for multiple dimensions (though perhaps incorrectly). I still don't really understand the case, so I'm not really sure what I'm supposed to be testing for. So I didn't add any more tests and thus can't really be sure that it's currently right for multiple dimensions.

jmh530 · 2017-08-05T02:27:54Z

@9il I've tried adding some more unittests (and some other small changes). Hopefully moving in the right direction.

As an aside, I have done some working with a function that formats an ndslice into a string (same format as the pretty versions I added here). At present I am making use of some features of the standard library and there is some use of the @gc. Does it have a place in mir-algorithm? Perhaps numir?

9il · 2017-08-07T03:46:40Z

source/mir/ndslice/topology.d

+ import mir.ndslice.internal : DimensionsCountCTError, DimensionCTError;
+
+ mixin DimensionsCountCTError;
+


just return slice.adjTransposed!Dimensions.ipack!(Dimensions.length);

please add a small unittest to cover Dimensions.length > 1

I had actually included a two-dimensional unittest on line 4412.

9il · 2017-08-07T03:47:55Z

source/mir/ndslice/topology.d

+}
+
+///
+auto byDim(SliceKind kind, size_t[] packs, Iterator)


I do not think we need this overload, because it is equal to ipack!0

This was to allow writing the default x.byDim instead of x.byDim!0

9il · 2017-08-08T05:29:25Z

As an aside, I have done some working with a function that formats an ndslice into a string (same format as the pretty versions I added here). At present I am making use of some features of the standard library and there is some use of the @gc. Does it have a place in mir-algorithm? Perhaps numir?

Mir Algorithms need formatting, and it may be based on 100% generic @nogc code from Phobos. GC usage is allowed by user, but @nogc allocators should be supported by API.

For GC based code numic seems to be a good front-end library.

Thank you for byDim PR, good addition!

jmh530 · 2017-08-08T14:35:29Z

@9il I think I could make progress on a @nogc version if issue #48 is resolved. The @gc part of my code is coming from std.array's join and appender.

And I'm glad to help out. I feel like I'm getting a little better understanding of what you're doing here.

9il · 2017-08-08T16:31:16Z

@9il I think I could make progress on a @nogc version if issue #48 is resolved. The @gc part of my code is coming from std.array's join and appender.

If you have ability to fix #48 this would be awesome!

Phobos appender allocates GC struct on the heep. So it behaves as it is an object. Other approach is RefCounted appender.

Third approach is an appender with disabled this(this) constructor. This is fastest and simplest approach and it will work on 99.99% cases (it should be passed by ref). Plus it can be used to implement GC-allocated and ref-counted versions later.

With allocator based appender we have the following API issue:

struct Appender(Allocator) if (hasReallocate!Allocator ... ) { Allocator allocator; }; struct Appender(Allocator) if (hasReallocate!Allocator ... ) { Allocator* allocator; }; struct Appender(Allocator) if (hasReallocate!Allocator ... ) { /+Uses Allocator.instance+/ };

The last variant is valid only for some allocators like Mallocator and GCAllocator. The first may not be valid for some allocators (it is not specified yet, I just realize this issue, that allocators should have disabled this(this) too). The second variant is most universal.

This is draft:

struct Appender(T Allocator) if (hasReallocate!Allocator && ...) { @disable this(this); private T[] _buffer; private size_t _length; static if (!hasInstance!Allocator) { private Allocator* _allocator; pragma(inline, true) private ref Allocator allocator()() { return *_allocator; } this(ref allocator) { _allocator = &allocator; // allocate buffer with say, 8 elements } } else { pragma(inline, true) private ref Allocator allocator()() { return Allocator.instance; } this(ref allocator) { // allocate buffer with say, 8 elements } } T[] data()() { return _buffer[0 .. _length]; } void put()(auto ref T elem) { // ... } void put(Iterable)(Iterable iterable) if (is(ForeachType!Iterable : T)) { // ... } }

For now simple extend logic for put methods is OK (something like x2 multiplication). You can review/reuse Phobos code without any issue, we only will need to add proper credentials and list all authors.

Also, we need our emplace realization or we can use Phobos for now, or restrict types that can be used by Appender (builtin types, classes, delegates, and structs without elaborate assign http://dlang.org/phobos/std_traits.html#hasElaborateAssign).

jmh530 · 2017-08-08T16:43:21Z

@9il I'm not sure that I have the skills to get appender working properly (was just saying that if it did work, then I could do a @nogc format for slices). I'd say as a first step, I would need to re-write it to rely less on strings and more on ndslices. Then I would need to change use of std.format.format to std.format.sformat. I think only then would I check to see if I can get the appender working.

9il · 2017-08-08T16:47:58Z

sformat throws :/ I like to have nothrow @nogc version of format at mir, it is large work to do from scratch, but we can use Phobos code, and merge our code back to phobos one day.

jmh530 · 2017-08-08T17:06:54Z

Gotcha.

Add byDim and apply functions

c4b7666

jmh530 force-pushed the jmh530-byDim branch from d7ed096 to c4b7666 Compare August 2, 2017 02:29

9il reviewed Aug 2, 2017

View reviewed changes

jmh530 changed the title ~~Add byDim and apply functions~~ Add byDim function Aug 2, 2017

9il reviewed Aug 2, 2017

View reviewed changes

Remove apply

94c239b

jmh530 force-pushed the jmh530-byDim branch from b333f9f to abf16e9 Compare August 3, 2017 02:13

Other fixes required

5c8651b

jmh530 force-pushed the jmh530-byDim branch from abf16e9 to 5c8651b Compare August 3, 2017 02:16

9il reviewed Aug 3, 2017

View reviewed changes

Fix fastmath issues and other issues

091a115

Fixes and flesh out unittests

a748e3c

9il reviewed Aug 7, 2017

View reviewed changes

Additional fixes

88409fc

jmh530 force-pushed the jmh530-byDim branch from 2604e9e to 88409fc Compare August 8, 2017 00:53

9il merged commit 521e138 into libmir:master Aug 8, 2017

jmh530 deleted the jmh530-byDim branch August 8, 2017 14:29


		mixin DimensionsCountCTError;

		static if (Dimensions.length == 1 && Dimensions[0] == 0)

		import mir.ndslice.internal : DimensionsCountCTError, DimensionCTError;

		mixin DimensionsCountCTError;

Add byDim function #79

Add byDim function #79

Uh oh!

Conversation

jmh530 commented Aug 1, 2017

codecov-io commented Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmh530 Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

9il commented Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

jmh530 commented Aug 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmh530 commented Aug 3, 2017

jmh530 commented Aug 5, 2017

Choose a reason for hiding this comment

9il Aug 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

9il commented Aug 8, 2017

jmh530 commented Aug 8, 2017

9il commented Aug 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

jmh530 commented Aug 8, 2017

9il commented Aug 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

jmh530 commented Aug 8, 2017

Labels

3 participants

codecov-io commented Aug 2, 2017 •

edited

Loading

jmh530 Aug 2, 2017 •

edited

Loading

9il commented Aug 2, 2017 •

edited

Loading

9il Aug 7, 2017 •

edited

Loading

9il commented Aug 8, 2017 •

edited

Loading

9il commented Aug 8, 2017 •

edited

Loading