seed ops #176

alpha-cactus · 2019-01-13T23:13:50Z

What does this PR do?

changes the random number generator for all ops using stdlib rand() to use libavr32 rand_state_t. add SEED op to set seed for all ops using rand. add separate ops to set seed for any individual op.

list of all added ops including aliases:
SEED
RAND.SEED / RAND.SD / R.SD
TOSS.SEED / TOSS.SD
PROB.SEED / PROB.SD
DRUNK.SEED / DRUNK.SD
P.SEED / P.SD

the algorithm used by rand_state_t comes from here(page 10):
numerical recipes

Provide links to any related discussion on lines.

How should this be manually tested?

I've been running these ops through multiple scripts the past few weeks. also wrote a simple teletype script to test that the seed setting functionality works. confirmed the stream of number generated is the same when setting the same seed.

only issue I found in testing is that the LSB oscillates between 0 and 1. I had to update the TOSS op to test bit 4 because of this. note this does cause something like the following to oscillate between the min/max number:
RRAND 8 9
will flip between returning 8 and 9. I understand this may be a serious concern. if so I can work on finding a better pnrg algorithm that can be seeded.

Any background context you want to provide?

added new file seed.c to house seed ops. added macro MAKE_SEED_OP for creating seed ops in 1 line. the macro functions similar to the MAKE_SIMPLE_VARIABLE_OP.

because all the seed ops are using the same function aliases are also made using the MAKE_SEED_OP macro.

used a union for housing the random_state_t structures for each op. this allows them to be accessed individually by nam, or iterated over for initialization and the SEED op.

If the related Github issues aren't referenced in your commits, please link to them here.

I have,

updated CHANGELOG.md
updated the documentation
run make format on each commit

…ake seed macro

…for accessing rand_states, updated seed and init functions for rand_states

…_state_t random_next call

"merge upstream"

alpha-cactus · 2019-01-14T00:20:38Z

also wanted to note this PR relies on this change made to libavr 32:
libavr32

I noticed that the teletype libavr32 still links to the version before this change. not exactly sure how recursive repositories work.

scanner-darkly · 2019-01-15T16:27:38Z

src/match_token.rl

+        "DRUNK.SD"		=> { MATCH_OP(E_OP_SYM_DRUNK_SD); };
+        "P.SEED"      => { MATCH_OP(E_OP_P_SEED); };
+        "P.SD"				=> { MATCH_OP(E_OP_SYM_P_SD); };
+


indentation seems off?

strange. I ran git-clang-format. will investigate when I get home later.

ya looks entirely fine on my end(running vim).

it's likely due to tabs being used which i think clang-format is supposed to convert (teletype uses 4 spaces convention)

this is what i'm seeing on github:

oh okay I think I'm using 2 spaces because that's norns convention.

scanner-darkly · 2019-01-15T16:38:18Z

src/ops/seed.c

+const tele_op_t op_DRUNK_SEED			= MAKE_SEED_OP(DRUNK.SEED, rand_states.s.drunk);  
+const tele_op_t op_SYM_DRUNK_SD		= MAKE_SEED_OP(DRUNK.SD, rand_states.s.drunk);  
+const tele_op_t op_P_SEED					= MAKE_SEED_OP(P.SEED, rand_states.s.pattern);  
+const tele_op_t op_SYM_P_SD				= MAKE_SEED_OP(P.SD, rand_states.s.pattern);


indentation seems off?

yup probably some sort of tab issue. will fix.

scanner-darkly · 2019-01-15T16:39:44Z

src/ops/op.c

+                      exec_state_t *NOTUSED(es), command_state_t *cs) {
+    char *base = (char *)ss;
+    size_t offset = (size_t)data;
+    rand_set_t *ptr = (rand_set_t *)(base + offset);


does this depend on specific order of members in scene_state_t?

no I don't think order matters. I'm using the same technique as the MAKE_SIMPLE_VARIABLE_OP macro, but the warning comment for variable order is related to something else.

https://llllllll.co/t/teletype-firmware-discussion/13961/38?u=alphacactus

src/state.h

scanner-darkly · 2019-01-15T16:48:55Z

great, added some comments!

re: libavr32 - i assume you want to update it to monome/libavr32@377bc18? to do so go to libavr32 folder and do a git fetch and check out the commit you need. once you do this it will show up as a change in your teletype repo, and you can commit it as a regular commit (what it will do is basically update the commit the teletype repo is referencing).

for more info on working with submodule check out the guide @samdoshi put together: https://samdoshi.com/post/2016/03/git-submodules/

alpha-cactus · 2019-01-15T19:21:16Z

src/ops/op.c

@@ -240,6 +247,26 @@ void op_poke_i16(const void *data, scene_state_t *ss, exec_state_t *NOTUSED(es),
    tele_vars_updated();
 }

+void op_peek_seed_i16(const void *data, scene_state_t *ss,


actually can probably move these and the macros into seed.c

…m random_init

alpha-cactus · 2019-01-16T00:30:20Z

only issue I found in testing is that the LSB oscillates between 0 and 1. I had to update the TOSS op to test bit 4 because of this. note this does cause something like the following to oscillate between the min/max number:
RRAND 8 9
will flip between returning 8 and 9 at random. I understand this may be a serious concern. if so I can work on finding a better pnrg algorithm that can be seeded.

still concerned about this.

scanner-darkly · 2019-01-16T00:32:40Z

not sure i fully understand - are you saying it doesn't respect the seed?

alpha-cactus · 2019-01-16T00:32:54Z

only issue I found in testing is that the LSB oscillates between 0 and 1. I had to update the TOSS op to test bit 4 because of this. note this does cause something like the following to oscillate between the min/max number:
RRAND 8 9
will flip between returning 8 and 9. I understand this may be a serious concern. if so I can work on finding a better pnrg algorithm that can be seeded.

actually don't think I was clear in my original post. output would look like this.

which probably isn't good. if we can eliminate the LSB from the equation it should be fine. maybe a bit SHIFT and OR? although at that point is it worth the trouble.

alpha-cactus · 2019-01-16T00:33:36Z

ya sorry post got cut off early.

scanner-darkly · 2019-01-16T00:35:30Z

ah i see, so the least significant bit is not random. does this apply to bigger ranges too? say if i did RRND 0 9 would it return odd/even/odd/even/odd etc?

alpha-cactus · 2019-01-16T00:40:10Z

yes that's why I had to change TOSS to test a different bit. probably wouldn't be hard to find a better tiny prng algorithm with some research as long as the rest of the code holds up to review.

scanner-darkly · 2019-01-16T00:43:43Z

yeah we could try right shifting it by 1 or XORing 2 numbers but that might also affect the quality of the algorithm..

alpha-cactus · 2019-01-16T00:48:29Z

ya looking at it now probably premature for this PR given the issue. I guess I wanted to see what others had to say on it. can probably close this for now and just have a single SEED op using stdlib srand() until I come up with something better.

scanner-darkly · 2019-01-16T00:50:30Z

let's leave it open - these are fantastic ops and very useful, we just need to find a better algo!
so if i understand correctly the issue with using srand is that you only get one seed?

alpha-cactus · 2019-01-16T01:06:32Z

let's leave it open - these are fantastic ops and very useful, we just need to find a better algo!

ok cool I had already been looking at a few should be able to come up with something before the weekend.

so if i understand correctly the issue with using srand is that you only get one seed?

yes that's my understanding of it.

scanner-darkly · 2019-01-16T02:00:06Z

in this case one way would be to reproduce the implementation but with the ability to maintain several states/seeds, and add an index parameter to rand/srand

alpha-cactus · 2019-01-16T13:40:01Z

oh ya hadn't really considered that! I'll look into it if not just to see how it works.

scanner-darkly · 2019-01-16T17:13:30Z

doing a quick search, this: http://sourceware.org/git/?p=glibc.git;a=blob;f=stdlib/random_r.c;hb=glibc-2.15#l361 has the ability to switch states. also this: http://www.cse.yorku.ca/~oz/marsaglia-rng.html has several algos that might work?

alpha-cactus · 2019-01-16T17:54:01Z

yes! didn't have much time last night, but I literally book marked those exact two pages for reading today.

alpha-cactus · 2019-01-17T01:56:31Z

code is a little dirty, but I think the Marsaglia KISS algorithm seems sufficient. still uses a Linear Congruential Generator like libavr32 so it can be seeded. however the MWC and SHR3 algorithms improve the randomness of the lower bits. I'll work it in to the teletype firmware if it looks good to you.

initial testing looks good. from what I can tell.
https://github.com/alpha-cactus/prng/blob/master/tele_rand.c

scanner-darkly · 2019-01-17T17:04:58Z

i only have basic knowledge about random generators, so i'm good with whatever you think will work best!

…sues

alpha-cactus · 2019-01-19T20:11:01Z

alright this should be the final revision. tele_rand_t uses the MWC(multiply with carry) prng. it returns a 32bit signed integer between 0x0 and 0x7FFFFFFF which is the same range as what the stdlib rand() call returns.

I could move tele_rand.c/h stuff to the seed.c/h files if you want.

give me a few days to run this through some tests before merging.

scanner-darkly · 2019-01-20T22:31:31Z

src/ops/seed.c

+                        command_state_t *cs);
+
+// clang-format off
+const tele_op_t op_SEED			= MAKE_GET_SET_OP(SEED, op_SEED_get, op_SEED_set, 0, true);


indentation off

scanner-darkly · 2019-01-20T22:32:44Z

src/state.h

@@ -96,7 +98,8 @@ typedef struct {
    uint8_t time_act;
    int16_t tr[TR_COUNT];
    int16_t tr_pol[TR_COUNT];
-    int16_t tr_time[TR_COUNT];
+	int16_t tr_time[TR_COUNT];


indentation off?

ugh. fixed what was causing this, so it shouldn't be an issue going forward. I went through my previous commits & pull requests and have hopefully fixed everything now. must've missed this one.

looks like indentation is still off here and on the next line

scanner-darkly · 2019-01-20T22:40:52Z

src/ops/maths.c

                        exec_state_t *NOTUSED(es), command_state_t *cs) {
-    cs_push(cs, rand() & 1);
+    tele_rand_t *r = &ss->rand_states.s.toss;
+    cs_push(cs, (tele_rand(r) >> 3) & 1);


do we still need to use a different bit with the latest algo?

probably not. I think the issue with less random lower bits is only tied to the linear congruential generator algorithm. I'll run some tests using the new algorithm with the lsb to verify.

scanner-darkly · 2019-01-20T22:42:32Z

added some minor comments, other than that looks good!
re: tele_rand.c/h - i think libavr32 might be a better place for it, since then other firmwares can take advantage of it too.

alpha-cactus · 2019-01-20T23:30:29Z

added some minor comments, other than that looks good!
re: tele_rand.c/h - i think libavr32 might be a better place for it, since then other firmwares can take advantage of it too.

ya I'm curious how many firmwares(if any) are using the current libavr32 random. I'd prefer updating it rather than adding a second random function to the library. should be simple enough to just replace the algorithm without changing any of the other functionality.

scanner-darkly · 2019-01-21T20:28:45Z

i did a search in trilogy/ansible/aleph repos and looks like the only thing that uses random_next is arp_seq_build_random which is used by arp_seq_build (both are in libavr32/src/arp.c) which is only used by ansible. so yeah i think replacing it with the new implementation should be fine.

alpha-cactus · 2019-01-24T03:05:11Z

@scanner-darkly
algorithm tests both manual/automated seem fine so far. wanted to get your opinion on the functionality of random_state_t. currently it can be instantiated with a min/max range, but the teletype firmware already handles that. so I'm debating whether to remove it as part of the libavr32 change.

link to my current version:
https://github.com/alpha-cactus/libavr32/blob/master/src/random.c
previous version:
https://github.com/monome/libavr32/blob/master/src/random.c

new version would leave range limiting responsibility to the caller. I could keep the range functionality, but would want to then probably remove it from the teletype firmware where possible since modulus operations are kind of expensive if I recall.

scanner-darkly · 2019-01-27T18:19:44Z

i would leave it in, unless that involves more changes, otherwise we'll have to check if anything uses it (and i don't think modulus would be a huge concern performance wise in the big scheme of things). why would you need to remove it from teletype though? so it doesn't do it twice?

alpha-cactus · 2019-01-27T19:10:39Z

well the libavr32 algorithm generates a number in the range 0x0-0x7FFFFFF. then that number is cut down to the specified min/max range and returned to teletype. in the case of an op like RRAND the returned value would then again be cut down to the range specified by the parameters the user puts in.

ya I was thinking maybe we could have libavr32 just perform all the range limiting. however, that might just reduce code readability/maintainability for what ends up being a really small optimization.

scanner-darkly · 2019-01-27T19:31:39Z

wouldn't libavr32 range limiting apply to all generated numbers? so you'd still need to apply it for RRND. so for teletype it doesn't make sense to use it, but i'm not sure if any other firmware that uses libavr32 does or not.

alpha-cactus · 2019-01-27T20:03:51Z

right it's currently built into the random_next function so there's no option to not specify a min/max range. I agree it doesn't make sense to perform a min/max range mod twice. currently as far as I can tell the only other firmware that uses random_next is the libavr32 arp.c. it would be relatively simple to modify arp.c if I were to get rid of the range mod from random_next.

https://github.com/monome/libavr32/blob/377bc181a81ed616647f2cb644f662711238e579/src/random.c#L26

alpha-cactus · 2019-02-07T00:39:50Z

ran some quick tests to confirm the libavr32 integration is good. bit of a wild PR thanks for sticking with it.

scanner-darkly · 2019-02-07T00:42:16Z

thanks for doing it!
@tehn - did you want to take a look?

alpha-cactus · 2019-02-08T03:59:08Z

I think the indentation is actually still off...

let me fix that first.

edit:
actually maybe it's fine? looked off on my phone.

scanner-darkly · 2019-03-27T02:10:37Z

ah sorry, didn't see your edit of the previous message - github doesn't send notification for edits. yeah indentation looks fine to me except a couple of lines, i left a comment there.

tehn

fantastic!

alpha-cactus added 8 commits Dec 16, 2018

first steps for seed ops

6129711

updated seed ops, scene seed init, new structure for random states, m…

1593fc6

…ake seed macro

updates scene_rand_t to use a union for array/struct access

d5e5c46

updates naming convention for rand_states, improved code readability …

cf36ddb

…for accessing rand_states, updated seed and init functions for rand_states

update TOSS to test different bit, modified push_random to use random…

bd70675

…_state_t random_next call

Merge remote-tracking branch 'upstream/master'

185575a

"merge upstream"

updated random seed op name

90e5ba0

move seed ops to new file, add aliases, add docs

Loading status checks…

2e43ff8

scanner-darkly reviewed Jan 15, 2019

View changes

src/state.h Outdated Show resolved Hide resolved

alpha-cactus reviewed Jan 15, 2019

View changes

move seed helpers from op.c/h to seed.c, remove unesscessary init fro…

Loading status checks…

bf5684b

…m random_init

alpha-cactus closed this Jan 16, 2019

alpha-cactus reopened this Jan 16, 2019

alpha-cactus added 2 commits Jan 18, 2019

add tele_rand_t, fix indentation, TODO fix push_random

Loading status checks…

f65d2ef

update tele rand return type and function names, fixed indentation is…

Loading status checks…

d3ecfa1

…sues

scanner-darkly reviewed Jan 20, 2019

View changes

src/ops/seed.c

command_state_t *cs);

// clang-format off

const tele_op_t op_SEED = MAKE_GET_SET_OP(SEED, op_SEED_get, op_SEED_set, 0, true);

This comment has been minimized.

Sign in to view

scanner-darkly Jan 20, 2019
Member

indentation off

scanner-darkly reviewed Jan 20, 2019

View changes

revert to use libavr32 random_state_t, todo maybe remove push_random

Loading status checks…

7d43fc0

alpha-cactus mentioned this pull request Jan 27, 2019

update libavr32 random number generator monome/libavr32#44

Merged

update random state init function for libavr32 change

Loading status checks…

ea17e00

scanner-darkly approved these changes Feb 7, 2019

View changes

tehn approved these changes Mar 27, 2019

View changes

tehn merged commit 75eda8b into monome:master Mar 27, 2019
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

monome / teletype

seed ops #176

seed ops #176

Conversation

alpha-cactus commented Jan 13, 2019 • edited

What does this PR do?

Provide links to any related discussion on lines.

How should this be manually tested?

Any background context you want to provide?

If the related Github issues aren't referenced in your commits, please link to them here.

I have,

alpha-cactus commented Jan 14, 2019 • edited

This comment has been minimized.

scanner-darkly Jan 15, 2019 • edited Member

This comment has been minimized.

alpha-cactus Jan 15, 2019 Author Contributor

This comment has been minimized.

alpha-cactus Jan 16, 2019 Author Contributor

This comment has been minimized.

scanner-darkly Jan 16, 2019 Member

This comment has been minimized.

scanner-darkly Jan 16, 2019 Member

This comment has been minimized.

alpha-cactus Jan 16, 2019 Author Contributor

This comment has been minimized.

scanner-darkly Jan 15, 2019 • edited Member

This comment has been minimized.

alpha-cactus Jan 15, 2019 Author Contributor

This comment has been minimized.

scanner-darkly Jan 15, 2019 Member

This comment has been minimized.

alpha-cactus Jan 15, 2019 Author Contributor

scanner-darkly commented Jan 15, 2019

This comment has been minimized.

alpha-cactus Jan 15, 2019 Author Contributor

alpha-cactus commented Jan 16, 2019

scanner-darkly commented Jan 16, 2019

alpha-cactus commented Jan 16, 2019 • edited

alpha-cactus commented Jan 16, 2019

scanner-darkly commented Jan 16, 2019

alpha-cactus commented Jan 16, 2019

scanner-darkly commented Jan 16, 2019

alpha-cactus commented Jan 16, 2019 • edited

scanner-darkly commented Jan 16, 2019 • edited

alpha-cactus commented Jan 16, 2019

scanner-darkly commented Jan 16, 2019

alpha-cactus commented Jan 16, 2019

scanner-darkly commented Jan 16, 2019

alpha-cactus commented Jan 16, 2019

alpha-cactus commented Jan 17, 2019 • edited

scanner-darkly commented Jan 17, 2019

alpha-cactus commented Jan 19, 2019 • edited

This comment has been minimized.

scanner-darkly Jan 20, 2019 Member

This comment has been minimized.

scanner-darkly Jan 20, 2019 Member

This comment has been minimized.

alpha-cactus Jan 20, 2019 Author Contributor

This comment has been minimized.

scanner-darkly Mar 27, 2019 • edited Member

This comment has been minimized.

scanner-darkly Jan 20, 2019 Member

This comment has been minimized.

alpha-cactus Jan 20, 2019 • edited Author Contributor

scanner-darkly commented Jan 20, 2019

alpha-cactus commented Jan 20, 2019

scanner-darkly commented Jan 21, 2019

alpha-cactus commented Jan 24, 2019

scanner-darkly commented Jan 27, 2019

alpha-cactus commented Jan 27, 2019

scanner-darkly commented Jan 27, 2019

alpha-cactus commented Jan 27, 2019

alpha-cactus commented Feb 7, 2019

scanner-darkly commented Feb 7, 2019

alpha-cactus commented Feb 8, 2019 • edited

scanner-darkly commented Mar 27, 2019

tehn left a comment

alpha-cactus commented Jan 13, 2019 •

edited

alpha-cactus commented Jan 14, 2019 •

edited

scanner-darkly Jan 15, 2019 •

edited

Member

alpha-cactus Jan 15, 2019
Author Contributor

alpha-cactus Jan 16, 2019
Author Contributor

scanner-darkly Jan 16, 2019
Member

scanner-darkly Jan 16, 2019
Member

alpha-cactus Jan 16, 2019
Author Contributor

scanner-darkly Jan 15, 2019 •

edited

Member

alpha-cactus Jan 15, 2019
Author Contributor

scanner-darkly Jan 15, 2019
Member

alpha-cactus Jan 15, 2019
Author Contributor

alpha-cactus Jan 15, 2019
Author Contributor

alpha-cactus commented Jan 16, 2019 •

edited

alpha-cactus commented Jan 16, 2019 •

edited

scanner-darkly commented Jan 16, 2019 •

edited

alpha-cactus commented Jan 17, 2019 •

edited

alpha-cactus commented Jan 19, 2019 •

edited

scanner-darkly Jan 20, 2019
Member

scanner-darkly Jan 20, 2019
Member

alpha-cactus Jan 20, 2019
Author Contributor

scanner-darkly Mar 27, 2019 •

edited

Member

scanner-darkly Jan 20, 2019
Member

alpha-cactus Jan 20, 2019 •

edited

Author Contributor

alpha-cactus commented Feb 8, 2019 •

edited