-
-
Notifications
You must be signed in to change notification settings - Fork 103
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
Conversation
Quick tests: Also, stack(
s("hh*8").degrade(),
s("[ht*8]?")
) is still in sync, which is also the case on Evaluating |
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 However, on my branch, I don't observe an even spread amongst the four options as I'd expect, but instead I see that 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 For the time being, I'll just change |
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 But for now, I'd say this could be merged as is, as it makes randomness much more usable! |
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 topatternifyAST()
, 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.