Add eachTriangle #95

jmh530 · 2017-09-26T00:11:35Z

After some of the work on eachUploPair I was thinking that it might make sense to specialize a version for upper and lower triangles. This would be useful for implementing functions similar to tril and triu from Matlab/Numpy in numir.

I'm not sure this is the most efficient version, but at the moment it gets the job done and works on rectangular matrices the way I would expect.

thewilsonator · 2017-09-26T01:12:52Z

source/mir/ndslice/algorithm.d

@@ -1884,3 +1885,760 @@ size_t countImpl(alias fun, Slices...)(Slices slices)
    while(!slices[0].empty);
    return ret;
 }
+
+private template eachLowerTriangle(alias fun, size_t k = 1,
+                                                bool BeyondMainDiagonal = false)


I think it would be better to have eachLowerTriangle(alias fun,ptrdiff_t diagonalOffset) if I am understanding this correctly.

That's more-or-less how it originally was, but I thought the code was a little simpler when I had done it this way.

thewilsonator · 2017-09-26T01:18:10Z

source/mir/ndslice/algorithm.d

+unittest
+{
+    import mir.ndslice.allocation: slice;
+    import mir.ndslice.topology: iota, canonical;


Is it possible to combine these essentially duplicate tests with an alias seq?
Something like

import mir.ndslice.topology: iota, canonical, universal; /*static*/ foreach(type; AliasSeq!(identity, canonical, universal) { auto m = iota([3, 3], 1).slice.type; m.eachLowerTriangle!((ref a) {a = 0; }, 0); assert(m == [ [0, 2, 3], [0, 0, 6], [0, 0, 0]]);

Yes, please. I like unittests, but it is nice to keep them smaller with the same coverage.

@thewilsonator The way I was able to get it to work looks like

version(unittest) { T identity(T)(T x) @property { return x; } } unittest { import mir.ndslice.allocation: slice; import mir.ndslice.topology: iota, canonical, universal; import std.meta: AliasSeq; void test(alias func)() { // 1 2 3 // 4 5 6 // 7 8 9 auto m = func(iota([3, 3], 1).slice); m.eachUpper!((ref a) {a = 0; }, 0); assert(m == [ [0, 0, 0], [4, 0, 0], [7, 8, 0]]); } static foreach(type; AliasSeq!(identity, canonical, universal)) { test!type; } }

9il · 2017-09-26T02:41:13Z

I'm not sure this is the most efficient version, but at the moment it gets the job done and works on rectangular matrices the way I would expect.

eachUploPair at master should be slightly faster then current implementation of eachTriangle. Furthermore, if one use a lambda with refs (ref u, ref l) then it will be optimised well (without refs for primitive types too). Plus, eachUploPair!((ref u, ref l){ ... }) has clearer API.

I do not think we need this API. Maybe docs for eachUploPair!((ref u, ref l){ ... }) can be updated.

jmh530 · 2017-09-26T02:56:44Z

@9il A few reasons I wrote this up: 1) eachUploPair only works for square matrices. This works for rectangular. 2) While eachUploPair works well for the main diagonal or one off the main diagonal, it is a little awkward to express further shifts in the diagonal. I honestly didn't even try it.

This function makes it very easy to implement the triu and tril functions as they are in Matlab/Numpy. They are currently not implemented in numir. I think it would be more difficult with eachUploPair, even if eachUploPair worked for rectangular matrices.

9il · 2017-09-26T03:07:00Z

OK, I see.
Please compress unittests where possible.

What about eachUpper and eachLower names?

jmh530 · 2017-09-26T03:12:53Z

I'm ok with that. I can also add specialization for the square cases where eachUploPair can be used.

…

On Mon, Sep 25, 2017 at 11:07 PM Ilya Yaroshenko ***@***.***> wrote: OK, I see. Please compress unittests where possible. What about eachUpper and eachLower names? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#95 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMJWyhjguOXEd-IRR5PZhtuyAA2HtyA0ks5smGpUgaJpZM4PjgSQ> .

9il · 2017-09-26T03:28:17Z

I can also add specialization for the square cases where
eachUploPair can be used.

~~Probably implementation should look like calls of eachUploPair and eachImpl~~

9il · 2017-09-26T09:33:20Z

@jmh530 BeyondMainDiagonal looks useless (popFront!0 / popFront!1 can be used instead).

Possibly eachUpper and eachLower should accept multiple arguments as each do.

jmh530 · 2017-09-26T11:10:49Z

@9i With respect to BeyondMainDiagonal I was planning to make the adjustment suggested by @thewilsonator above.

9il · 2017-09-26T11:27:46Z

@9i With respect to BeyondMainDiagonal I was planning to make the adjustment suggested by @thewilsonator above.

LGTM

codecov-io · 2017-09-27T01:46:30Z

Codecov Report

Merging #95 into master will increase coverage by 1.38%.
The diff coverage is 98.88%.

@@            Coverage Diff             @@
##           master      #95      +/-   ##
==========================================
+ Coverage   95.24%   96.62%   +1.38%     
==========================================
  Files          45       30      -15     
  Lines        6599     5456    -1143     
==========================================
- Hits         6285     5272    -1013     
+ Misses        314      184     -130

Impacted Files	Coverage Δ
source/mir/ndslice/package.d	`92.25% <ø> (ø)`	⬆️
source/mir/ndslice/algorithm.d	`99.09% <98.88%> (-0.09%)`	⬇️
source/mir/ndslice/internal.d	`57.69% <0%> (-1.96%)`	⬇️
source/mir/ndslice/chunks.d	`95.62% <0%> (-0.68%)`	⬇️
source/mir/ndslice/iterator.d	`95.12% <0%> (-0.6%)`	⬇️
source/mir/ndslice/topology.d	`99.28% <0%> (-0.55%)`	⬇️
source/mir/functional.d	`96.34% <0%> (-0.35%)`	⬇️
source/mir/ndslice/concatenation.d	`99.03% <0%> (-0.01%)`	⬇️
source/mir/math/sum.d	`99.4% <0%> (-0.01%)`	⬇️
... and 33 more

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 3cd51f4...d95cf16. Read the comment docs.

thewilsonator · 2017-09-27T01:52:30Z

LGTM. Resolve the conflict and make sure the CI passes. Wr.t the latter I'm not sure that LDC has static foreach yet, you may have to remove that, either way it will work without it.

9il · 2017-09-27T02:15:16Z

source/mir/ndslice/algorithm.d

+
+version(unittest)
+{
+    T identity(T)(T x)


no need in global declaration. can be replaced with lambda

I don't see how to do this with lambdas without making it more complicated. The lambdas I was trying weren't inferring the return type correctly, so then I tried making it a template and it just got uglier. I just put the identity function in each of the unittests instead of the version.

9il · 2017-09-27T02:15:56Z

source/mir/ndslice/algorithm.d

+    fun = A function
+    k = adjustment to diagonals (default = 1)
+/
+template eachLower(alias fun, ptrdiff_t k = 1)


why not to make k a runtime parameter?

I will make it a run-time parameter. The reason I hadn't is because I couldn't think of a time when I wanted to use triu or tril without knowing at compile-time how I was going to use it. Since this is more general than those functions, it probably makes sense.

As an aside, it would be much easier if in D you could have some parameters be available in run-time or compile-time.

9il · 2017-09-27T02:17:18Z

source/mir/ndslice/algorithm.d

+    static if (__traits(isSame, naryFun!fun, fun))
+    {
+        void eachLower(SliceKind kind, Iterator)
+                           (Slice!(kind, [2], Iterator) matrix)


Multiple arguments is required for many algorithms (the same way as each). For example, for triu implementation.

I didn't fully understand this, but it is late and I will try to look at it again with fresh eyes.

9il · 2017-09-27T02:17:57Z

source/mir/ndslice/algorithm.d

+
+            void eachLowerImpl(T)(T matrix)
+            {
+                void eachLowerImpl_i(U)(U e, size_t i)


No non-static inner functions plz

9il · 2017-09-27T02:31:41Z

source/mir/ndslice/algorithm.d

+                           (Slice!(kind, [2], Iterator) matrix)
+        {
+            immutable(size_t) m = matrix.length!0;
+            immutable(size_t) n = matrix.length!1;


Oh, lets replace the implementation with single one loop and without eachUploPair calls (sorry for wrong recommendation before, found algo issues with it now). We should be careful about code time generation and object code size.

x x x x < | 2D eachImpl x x x x < | x x x x < || Loop over 1D eachImpl x x x < || x x < || x < ||

I think I have implemented this in the latest commit.

9il · 2017-09-27T02:35:07Z

source/mir/ndslice/algorithm.d

+                    if (i < (n + k))
+                        e[0..(i - k + 1)].eachImpl!fun;
+                    else
+                        e.eachImpl!fun;


eachImpl probably will be inlined, so the following code will be two times smaller (in terms of object code size):

eachImpl!fun(i < n + k ? e[0..(i - k + 1)] : e);

EDIT: generally we should not have conditions in the loop. So dismiss this comment.

jmh530 · 2017-09-28T13:02:18Z

@9il I think I've got a handle on how to make it work for multiple slices now, but still a little work to do.

jmh530 · 2017-10-05T02:14:58Z

@9il I've submitted some updates. The only fail is on DMD nightly. Let me know if you want additional changes.

PetarKirov · 2017-10-05T07:40:25Z

The problem on Travis CI doesn't appear related to your pull request - the installer failed:

source "$(CURL_USER_AGENT="$CURL_USER_AGENT" bash install.sh dmd-nightly --activate)"
Warning: No gpg tool found to verify downloads.
curl: (56) Recv failure: Operation timed out
/Users/travis/.travis/job_stages: line 57: : No such file or directory

9il · 2017-10-07T08:17:34Z

source/mir/ndslice/algorithm.d

+                    foreach(ref slice; slices)
+                        slice.popFront!0;
+                    i++;
+                } while (i < k);


Looks like this loop can be replaced with slice.popFrontExactly!0(k)

9il · 2017-10-07T08:17:56Z

source/mir/ndslice/algorithm.d

+                val = i - k + 1;
+                static if (slices[0].shape.length == 1)
+                    mixin("fun(" ~
+                              frontSelectFrontOf!(Slices.length, "val") ~ ");");


please use single line here

Does mir follow a line length restriction? I normally use 80.

9il · 2017-10-07T08:18:00Z

source/mir/ndslice/algorithm.d

+                              frontSelectFrontOf!(Slices.length, "val") ~ ");");
+                else
+                    mixin(".eachImpl!fun(" ~
+                              frontSelectFrontOf!(Slices.length, "val") ~ ");");


9il · 2017-10-07T08:19:01Z

source/mir/ndslice/algorithm.d

+                                   selectFrontOf!(Slices.length, "val") ~ ");");
+                do
+                {
+                    foreach(ref slice; slices)


ditto popFrontExactly

9il · 2017-10-07T08:22:49Z

source/mir/ndslice/algorithm.d

+
+            static if ((Inputs.length > 1) && (isIntegral!(Inputs[$ - 1])))
+            {
+                k = inputs[$ - 1];


sizediff_t k = inputs[$ - 1];

What's sizediff_t?

9il · 2017-10-07T08:23:12Z

source/mir/ndslice/algorithm.d

+                alias slices = inputs[0..($ - 1)];
+            }
+            else
+            {


enum sizediff_t k = 1;

9il · 2017-10-24T04:10:36Z

source/mir/ndslice/package.d

@@ -166,6 +166,7 @@ $(TR $(TDNW $(SUBMODULE algorithm)
        $(SUBREF algorithm, cmp)
        $(SUBREF algorithm, count)
        $(SUBREF algorithm, each)
+        $(SUBREF algorithm, eachTriangle)


names was changed

9il · 2017-10-24T04:16:32Z

source/mir/ndslice/algorithm.d

+            function applied.
+        +/
+        void eachLower(Inputs...)(Inputs inputs)
+            if (Inputs.length)


Needs batter contraints:
(Inputs.length > 1) && (isIntegral!(Inputs[$ - 1])) || Inputs.length

9il · 2017-10-24T04:20:56Z

source/mir/ndslice/algorithm.d

+            if (k < 0)
+            {
+                val = -k;
+                static if (slices[0].shape.length == 1)


do we have 1D case?

9il · 2017-10-24T04:21:53Z

source/mir/ndslice/algorithm.d

+            import std.meta : allSatisfy;
+            import std.traits : isIntegral;
+            import mir.ndslice.traits : isMatrix;
+            import mir.ndslice.slice : Slice;


Is something from this imports are global in the module?

9il · 2017-10-24T04:25:25Z

source/mir/ndslice/algorithm.d

+    //| 5  6  7  8 |
+    //| 9 10 11 12 |
+    auto m = iota([3, 4], 1).slice;
+    m.eachUpper!((ref a) {a = 0; })(-2);


m.eachUpper!"a = 0" should work too. Please use it since it does not generate template bloat for each difinition.

jmh530 · 2018-02-07T01:45:41Z

@9il ...and I just realized that there were changes requested here. Sorry about that. I'll try to get back to this.

jmh530 · 2018-02-16T05:04:06Z

@9il I've made some updates. Travis-CI is failing on ldc-1.7.0 for some reason, but otherwise looks fine.

9il · 2018-02-16T09:25:45Z

awesome!

thewilsonator reviewed Sep 26, 2017

View reviewed changes

9il reviewed Sep 27, 2017

View reviewed changes

9il requested changes Sep 27, 2017

View reviewed changes

jmh530 force-pushed the jmh530-eachTriangle branch from 125e60b to 4ceeea6 Compare September 27, 2017 03:36

jmh530 force-pushed the jmh530-eachTriangle branch from 95c66bf to 4eadd12 Compare October 5, 2017 00:13

9il requested changes Oct 7, 2017

View reviewed changes

9il requested changes Oct 24, 2017

View reviewed changes

jmh530 added 9 commits February 15, 2018 22:24

Add eachTriangle

02ad1dd

Re-name eachUpper/Lower

0308951

Remove eachTriangle

30eda87

Compress UTs

5828a4d

Remove BeyondMainDiagonal

9ed7036

Add eachLower/Upper specialiation

3d308cd

Move identity to UT

9ed70b7

Remove inner functions/eachUploPair

9ea8c5f

Make k run-time parameter

f40646c

jmh530 added 15 commits February 15, 2018 22:27

Fix top documentation

3eb97fe

Adjust internal function structure

4c6178e

Add multiple slice support for eachLower/Upper

e1dbe89

Adjusting new templates

332e193

Add mir-test

ee878ec

Add UT attributes to eachLower/Upper

b126b26

Remove static foreach

a666807

Make requested changes

af2d933

Update documentation

966d2de

Fix package.d documentation

e6c7d12

Remove some eachLower/Upper imports

618205c

Improve eachLower/Upper template constraints

260aaa7

Change function in eachLower/Upper UT

8d0a773

Remove static if for 1-d slice

02f073b

Add enum for sizediff_t

a4557b4

jmh530 force-pushed the jmh530-eachTriangle branch from 8ac2135 to a4557b4 Compare February 16, 2018 03:33

Remove enum on sizediff_t

d95cf16

9il merged commit 402bcd2 into libmir:master Feb 16, 2018

jmh530 deleted the jmh530-eachTriangle branch February 16, 2018 13:02

Add eachTriangle #95

Add eachTriangle #95

Uh oh!

Conversation

jmh530 commented Sep 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

jmh530 commented Sep 26, 2017

Uh oh!

9il commented Sep 26, 2017

Uh oh!

jmh530 commented Sep 26, 2017 via email

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

jmh530 commented Sep 26, 2017

Uh oh!

9il commented Sep 26, 2017

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

thewilsonator commented Sep 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmh530 commented Sep 28, 2017

Uh oh!

jmh530 commented Oct 5, 2017

Uh oh!

PetarKirov commented Oct 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

9il commented Sep 26, 2017 •

edited

Loading

9il commented Sep 26, 2017 •

edited

Loading

9il commented Sep 26, 2017 •

edited

Loading

codecov-io commented Sep 27, 2017 •

edited

Loading

9il Sep 27, 2017 •

edited

Loading

9il Oct 24, 2017 •

edited

Loading