mir.sparse #79

9il · 2016-04-12T11:44:14Z

PR for tracking #43 implementation.

codecov-io · 2016-04-12T11:52:19Z

Current coverage is `96.55%`

Merging #79 into master will decrease coverage by -2.84% as of 679d422

@@            master     #79   diff @@
======================================
  Files            7      18    +11
  Stmts         2643    3223   +580
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit           2627    3112   +485
  Partial          0       0       
- Missed          16     111    +95

Review entire Coverage Diff as of 679d422

Uncovered Suggestions

+0.50% via .../model/lda/hoffman.d#145...160
+0.40% via .../model/lda/hoffman.d#165...177
+0.25% via .../model/lda/hoffman.d#61...68
See 7 more...

Powered by Codecov. Updated on successful CI builds.

9il · 2016-04-13T10:56:00Z

@wilzbach will use common hash map :-) initial implementation pushed to this branch

wilzbach · 2016-04-13T11:33:59Z

source/mir/ndslice/sparse.d

+    import mir.ndslice.slice: sliced;
+    T[size_t] table;
+    table[0] = 0;
+    table.remove(0);


why is that needed?

to initialize table with not null

but isn't that just a language feature? Or why do you need not null?

int[int] a; writeln(a.get(0, 2)); // 2

writeln(a.get(0, 2)); // 2

writeln(a) ?

assert(a !is null); ?

but isn't that just a language feature? Or why do you need not null?

For correct behavior when when coping

do you mind to add an unittest for the correct behavior? :)

wilzbach · 2016-04-13T13:06:56Z

@wilzbach will use common hash map :-) initial implementation pushed to this branch

I really like the direction this is going and the seamless integration with Slice. I would add a couple of unittests that this more, e.g.

mySparse[] = mySlice;
mySlice[] = mySparse;

and some of the iteration and selection algorithms

9il · 2016-04-13T13:08:31Z

no, please, let me finish it first. It is faster to implement this than to review api for a couple of small functions

wilzbach · 2016-04-13T13:09:36Z

no, please, let me finish it first

toggle annoying_bot off

9il · 2016-04-13T13:10:49Z

no, please, let me finish it first
toggle annoying_bot off

Have you start to collect article information for random distributions?

9il · 2016-04-18T17:48:27Z

It is almost covered and passes CI tests. I have added sparse_blas package and moved sum to the mir root.

wilzbach · 2016-04-18T17:53:03Z

source/mir/ndslice/sparse.d

+	}
+}
+
+private sizediff_t cmpCoo(size_t N)(const auto ref size_t[N] a, const auto ref size_t[N] b)


you only use this once (three lines above) - thus why not directly putting it there?

this may be used by the future algorithms

wilzbach · 2016-04-18T18:03:52Z

It is almost covered and passes CI tests. I have added sparse_blas package and moved sum to the mir root.

Wow! Looks great on my first pass. I will have to play with to give further feedback (will do so tonight).

Nitpicks from the first pass:

a lot of doc is missing
your editor is using tabs? Please configure it to use spaces ;-)
saw a couple of places with debug code

Do you plan to support the nogc code via Allocators?
This probably requires a custom implementation of hashmaps?

moved sum to the mir root.

you probably have to update the index.d too

9il · 2016-04-18T18:07:43Z

Do you plan to support the nogc code via Allocators?

Yes, but when it would be at phobos.

This probably requires a custom implementation of hashmaps?

Yes for Sparse and no for CompressedTensor.

9il · 2016-04-18T18:43:57Z

i moved sparse and blas to separate package, so we would not include it to ndslice. I just added few ptr methods to ndslice.

wilzbach · 2016-04-18T23:17:16Z

source/mir/ndslice/slice.d

+
+    /++
+    This constructor should be used only for integration with other languages or libraries such as Julia and numpy.
+    Params:


thanks for changing & documenting this :)

wilzbach · 2016-04-24T10:47:22Z

source/mir/internal/math.d

+
+	alias round = llvm_round;
+
+    alias fmuladd = llvm_fmuladd;


nitpick - you combine white space and tabs in here :/

9il added the New Package label Apr 12, 2016

9il mentioned this pull request Apr 12, 2016

mir.sparse multidimensional sparse arrays #43

Closed

9il force-pushed the sparse branch from 1159196 to d9c264a Compare April 12, 2016 12:06

wilzbach reviewed Apr 13, 2016
View reviewed changes

9il changed the title ~~mir.ndsparse~~ mir.ndslice.sparse Apr 15, 2016

9il force-pushed the sparse branch 4 times, most recently from a84bf47 to 1b9d2fd Compare April 18, 2016 15:06

wilzbach reviewed Apr 18, 2016
View reviewed changes

9il force-pushed the sparse branch from f1a72ec to 020aceb Compare April 18, 2016 18:11

9il changed the title ~~mir.ndslice.sparse~~ mir.sparse Apr 18, 2016

wilzbach reviewed Apr 18, 2016
View reviewed changes

9il added 20 commits April 22, 2016 07:51

remove debug code

c03a632

move code

72be68c

move code

5f6809f

fix names

2c295ff

make Iota private

6757add

move constructor in slice

dc34cc8

fix CompressedTensor

b89bd53

remove imports

cf0cabf

move to tabs

c42e984

remove useless doc from sum

99692c0

update docs

3899afe

move utilities

6e7a62b

add internal math

ceab4d4

add public math

83ac22e

update sparse

586bedf

add blas

3a11d5f

remove space

e1849d6

add constrints

ac45aef

update gemm

378a8d0

add readme

bbf9bb9

9il force-pushed the sparse branch from 9ccdb2f to bbf9bb9 Compare April 22, 2016 05:51

9il added 4 commits April 23, 2016 21:46

add LDA

1ffb2d1

Fix readme

29d366a

remove fmuladd usage

2e1f8de

update docs

072b736

9il merged commit 34b238b into master Apr 23, 2016

9il deleted the sparse branch April 23, 2016 20:45

wilzbach reviewed Apr 24, 2016
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mir.sparse #79

mir.sparse #79

9il commented Apr 12, 2016

codecov-io commented Apr 12, 2016

9il commented Apr 13, 2016

wilzbach Apr 13, 2016

9il Apr 13, 2016

wilzbach Apr 13, 2016

9il Apr 13, 2016

9il Apr 13, 2016

9il Apr 13, 2016

wilzbach Apr 13, 2016

9il Apr 13, 2016

wilzbach commented Apr 13, 2016

9il commented Apr 13, 2016

wilzbach commented Apr 13, 2016

9il commented Apr 13, 2016

9il commented Apr 18, 2016 •

edited

Loading

wilzbach Apr 18, 2016

9il Apr 18, 2016

wilzbach commented Apr 18, 2016

9il commented Apr 18, 2016

9il commented Apr 18, 2016

wilzbach Apr 18, 2016

wilzbach Apr 24, 2016

mir.sparse #79

mir.sparse #79

Conversation

9il commented Apr 12, 2016

codecov-io commented Apr 12, 2016

Current coverage is 96.55%

9il commented Apr 13, 2016

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

wilzbach commented Apr 13, 2016

9il commented Apr 13, 2016

wilzbach commented Apr 13, 2016

9il commented Apr 13, 2016

9il commented Apr 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilzbach commented Apr 18, 2016

9il commented Apr 18, 2016

9il commented Apr 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Current coverage is `96.55%`

9il commented Apr 18, 2016 •

edited

Loading