Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maintain random seed state in parser, not globally #531

Merged
merged 5 commits into from Mar 23, 2023

Conversation

ijc8
Copy link
Contributor

@ijc8 ijc8 commented Mar 20, 2023

Fixes #530.

An alternative approach would be to defer this until patternifyAST(), more like in @bpow's original version, in which seeds are generated at AST->pattern conversion time. I think this would work fine if the state was reset for each top-level call to patternifyAST(), which would also effectively result in per-parse state.
The approach I've taken here is more like how Tidal does it, where the seeds are generated at parse time.

@ijc8
Copy link
Contributor Author

ijc8 commented Mar 20, 2023

Quick tests:
s("[sd|cp] [sd|cp]") - previously made the same choice for both (sd sd or cp cp), now all four combinations occur
s("bd? sd?") - previously played both or neither; now can play one or the other
"[c3|e3|a3],[c4|e4|a4]".note() (from #165 (comment)) - previously played octaves, now plays various intervals

Also,

stack(
  s("hh*8").degrade(),
  s("[ht*8]?")
)

is still in sync, which is also the case on main (but presumably wasn't between #165 and #295).

Evaluating console.log(s("bd? sd?").queryArc(0, 1).map(e => e.show())) repeatedly always yields the same result, as parsing & pattern construction are free of side-effects (because random seeds are only incremented within each parse, not between parses).

@ijc8
Copy link
Contributor Author

ijc8 commented Mar 21, 2023

I tried writing a new test to confirm that all possibilities occur with equal probability... only to find that it's not the case!

Here's my experiment:

const numCycles = 1000;
const values = mini('[a|b] [a|b]').queryArc(0, numCycles).map(e => e.value);
const observed = { 'aa': 0, 'ab': 0, 'ba': 0, 'bb': 0 }
for (let i = 0; i < values.length; i += 2) {
    const chunk = values.slice(i, i + 2);
    observed[chunk.join('')]++;
}
console.log(observed);

Running on main, I observe { aa: 502, ab: 0, ba: 0, bb: 498 }; as expected, the alternating pairs never happen.

However, on my branch, I don't observe an even spread amongst the four options as I'd expect, but instead { aa: 344, ab: 158, ba: 151, bb: 347 }. All four combinations now occur, but there is still a strong bias towards the non-alternating combinations.

I see that rand only has a period of 300 cycles, so I suppose it's only the first 300 cycles that count for this; with numCycles = 300, I get { aa: 104, ab: 46, ba: 46, bb: 104 }.

For comparison, in Tidal the distribution is more even:

import Data.Maybe
import Data.List.Split
import qualified Data.Map as Map

pairs = chunksOf 2 $ concat $ map eventValue $ queryArc ("[a|b] [a|b]" :: Pattern String) (Arc 0 300)

foldr (\k m -> Map.alter (\n -> Just $ fromMaybe 0 n + 1) k m) Map.empty pairs

the last expression evaluates to

fromList [("aa",80),("ab",70),("ba",70),("bb",80)]

I wondered if the difference was due to randOffset (0.0001 in Tidal vs. 0.0002 in Strudel) and tried changing it. To my surprise, I found that the distribution was very sensitive to this value. At 0.0001 it's even worse ({ aa: 126, ab: 24, ba: 24, bb: 126 }) and at 0.0003 it's better ({ aa: 78, ab: 72, ba: 72, bb: 78 }).

For the time being, I'll just change randOffset to 0.0003 and add the new test.

@felixroos
Copy link
Collaborator

great to see you've managed to get it to work, thank you so much :) I am surprised that no tests failed... Probably because all test tunes (tunes.mjs) that use "|" or "?" are only tested for one cycle, and the first cycle is still the same.

In the future, we could think about if it makes sense to move that seed state from the mini parser to the randomness functions themselves, as using chooseInWith or degradeByWith without mini notation is still showing the stateless behavior.

But for now, I'd say this could be merged as is, as it makes randomness much more usable!

@felixroos felixroos merged commit ddf61a6 into tidalcycles:main Mar 23, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Random operators (| and ?) always make the same choice in a cycle
2 participants