Add std.random.discrete: Discrete sampler by wilzbach · Pull Request #234 · libmir/mir

wilzbach · 2016-06-25T22:29:34Z

9il · 2016-06-25T22:40:20Z

source/mir/random/mixture.d

+    size_t opCall(RNG)(ref RNG gen) const
+    {
+        import std.random : uniform;
+        T v = uniform!"[]"(0, _cdPoints[$-1], gen);


it can be just uniform

without "[]"

codecov-io · 2016-06-25T22:46:20Z

Current coverage is 96.52%

Merging #234 into master will increase coverage by 0.02%

@@             master       #234   diff @@
==========================================
  Files            18         19     +1   
  Lines          3230       3256    +26   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           3117       3143    +26   
  Misses          113        113          
  Partials          0          0

Powered by Codecov. Last updated by 03c87f6...c2f8930

9il · 2016-06-25T22:52:58Z

source/mir/random/mixture.d

+    size_t opCall(RNG)(ref RNG gen) const
+    {
+        import std.random : uniform;
+        T v = uniform(0, _cdPoints[$-1], gen);


uniform!T please

doesn't work - the prototype is auto uniform(string boundaries = "[)", T1, T2)(T1 a, T2 b)

https://dlang.org/phobos/std_random.html#.uniform

uniform!("[)", T, T) then

9il · 2016-06-25T22:58:39Z

ping @joseph-wakeling-sociomantic

- uint array instead of associative array - named Random variable - renamed probs -> cdPoints - reduced iteration size to 1000

wilzbach · 2016-06-25T23:49:34Z

Docs can be previewed at preview.mir.rocks.

ping @joseph-wakeling-sociomantic

@WebDrake you told me to ping both accounts?

9il · 2016-06-26T06:02:12Z

source/mir/random/mixture.d

+    A sampler that can be called to sample from the distribution.
+*/
+struct Discrete(T)
+    if (isNumeric!T)


Struct declaration should be after function. And add a small separate comment please

Struct declaration should be after function. And add a small separate comment please

ok for the order, but ditto would group them:

9il · 2016-06-26T09:42:49Z

source/mir/random/discrete.d

+    return Discrete!T(cdPoints);
+}
+
+/// ditto


Please do not compose function and struct declarations

WebDrake · 2016-06-26T11:05:33Z

source/mir/random/discrete.d

+    {
+        import std.random : uniform;
+        T v = uniform!("[)", T, T)(0, _cdPoints[$-1], gen);
+        return (cast(SortedRange!(const(T)[], "a <= b")) r).lowerBound(v).length;


Can't you just use assumeSorted here ... ?

SortedRange constructor always check that range is sorted in debug mode. This will affect complexity

... actually, isn't r already a sorted range? Why the cast at all?

Why the cast at all?

because the internal variable r has type const SortedRange!(const(T)[], "a <= b") - I didn't know another way to get rid of the const.

I would repeat my suggestion that you look at hap.random's DiscreteDistribution -- I did not need any casts under the hood:
https://github.com/WebDrake/hap/blob/e25fc15a17e4e439333740d2a9894fc7d3c94012/source/hap/random/distribution.d#L323-L329

opCall is const

Right, but that would mean you can't use lowerBound with a const SortedRange here, right? Surely that should be fixed in SortedRange, rather than covering up the non-const-ness with a cast?

Surely that should be fixed in SortedRange, rather than covering up the non-const-ness with a cast?

Yes I will try to fix Phobos, but that won't be released before 2.073 :/

Yes I will try to fix Phobos, but that won't be released before 2.073 :/

You have not time to fix Phobos templates, please consider on related issues

I'm not sure that it's appropriate to prioritize putting const on opCall above having clean and easy-to-follow code. Things like the cast and the two different internal variables for the cumulative distribution points create unnecessary complication in the code, and the cast in any case risks breaking the const guarantee (certainly if e.g. at some point you generalize the type which can be provided as input to the constructor).

This is a case where the fix needs to be upstream, and where the guarantee wanted here can easily be added after that fix is complete.

- renamed unittest variable rndGen -> gen - renamed r -> _cdSortedRange

- renamed unittest variable rndGen -> gen - renamed r -> _cdSortedRange - use assumeSorted instead of directly constructing the SortedRange

- renamed unittest variable rndGen -> gen - removed r -> in favor of using assumeSorted directly in opCall

* Adress comments from #234 - renamed unittest variable rndGen -> gen - removed r -> in favor of using assumeSorted directly in opCall * remove cdPoints API

wilzbach force-pushed the mixture branch 3 times, most recently from e0da7ef to d0c322e Compare June 25, 2016 22:33

9il reviewed Jun 25, 2016
View reviewed changes

Add std.random.mixture: Discrete sampler

eae1274

wilzbach force-pushed the mixture branch from d0c322e to eae1274 Compare June 25, 2016 22:52

9il reviewed Jun 25, 2016
View reviewed changes

wilzbach added 2 commits June 26, 2016 01:25

addressed comments

f1ec416

- uint array instead of associative array - named Random variable - renamed probs -> cdPoints - reduced iteration size to 1000

fix missing line in code coverage

ed7fb70

9il reviewed Jun 26, 2016
View reviewed changes

rename to discrete.d

75d7e4e

wilzbach changed the title ~~Add std.random.mixture: Discrete sampler - fixes #233~~ Add std.random.discrete: Discrete sampler Jun 26, 2016

wilzbach added 3 commits June 26, 2016 11:25

renaming to discrete - part 2

8f62f6b

Move struct declaration below function

04ed13b

use explicit template for uniform

2c9d511

9il reviewed Jun 26, 2016
View reviewed changes

source/mir/random/discrete.d Outdated

return Discrete!T(cdPoints);

}

/// ditto

Copy link

Member

9il Jun 26, 2016

Choose a reason for hiding this comment

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

Please do not compose function and struct declarations

more doc descriptions

49d6506

wilzbach added the New Package label Jun 26, 2016

wilzbach added 2 commits June 26, 2016 12:21

use std.algorithm.iteration and move unittest to function

c2f8930

add lref to Discrete struct

7562bb4

9il merged commit b6c255f into libmir:master Jun 26, 2016

WebDrake reviewed Jun 26, 2016
View reviewed changes

wilzbach added a commit to wilzbach/mir that referenced this pull request Jun 26, 2016

Adress comments from libmir#234

c3b0e8e

- renamed unittest variable rndGen -> gen - renamed r -> _cdSortedRange

wilzbach added a commit to wilzbach/mir that referenced this pull request Jun 26, 2016

Adress comments from libmir#234

9d30bf6

- renamed unittest variable rndGen -> gen - renamed r -> _cdSortedRange

wilzbach mentioned this pull request Jun 26, 2016

Address comments from #234 #235

Merged

wilzbach added a commit to wilzbach/mir that referenced this pull request Jun 26, 2016

Adress comments from libmir#234

4c8e91b

- renamed unittest variable rndGen -> gen - renamed r -> _cdSortedRange - use assumeSorted instead of directly constructing the SortedRange

wilzbach added a commit to wilzbach/mir that referenced this pull request Jun 26, 2016

Adress comments from libmir#234

d7d2255

- renamed unittest variable rndGen -> gen - removed r -> in favor of using assumeSorted directly in opCall

9il pushed a commit that referenced this pull request Jun 26, 2016

Address comments from #234 (#235)

220b2c8

* Adress comments from #234 - renamed unittest variable rndGen -> gen - removed r -> in favor of using assumeSorted directly in opCall * remove cdPoints API

wilzbach deleted the mixture branch June 26, 2016 13:47

Comments

Conversation

wilzbach commented Jun 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jun 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 96.52%

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 Jun 25, 2016

Uh oh!

wilzbach commented Jun 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 Jun 26, 2016 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

wilzbach commented Jun 25, 2016 •

edited

Loading

codecov-io commented Jun 25, 2016 •

edited

Loading

wilzbach commented Jun 25, 2016 •

edited

Loading

9il Jun 26, 2016 •

edited

Loading