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

Split model training from randomness #71

Closed
Diggsey opened this issue May 29, 2021 · 3 comments · Fixed by #72
Closed

Split model training from randomness #71

Diggsey opened this issue May 29, 2021 · 3 comments · Fixed by #72

Comments

@Diggsey
Copy link
Contributor

Diggsey commented May 29, 2021

Training the model with "learn" is relatively slow (much slower than generating new words) so ideally you could train it once, and then use the same model to generate several sequences with different seeds.

However, this is not possible with the current API, because the seed is supplied when you first construct the MarkovChain, and cannot be changed after that point.

AFAICT, the "learn()" function is completely deterministic (it does not use the RNG) so this is a suboptimal design.

I would suggest removing the rng field from the MarkovChain entirely, and instead pass the RNG when you construct the Words iterator. This way you can train a model once, and then generate several sequences of text with different seeds. This should also make the lipsum_words_from_seed function much faster, even when not using a custom model.

@mgeisler
Copy link
Owner

Hi @Diggsey,

Thanks for reporting this! Your analysis sounds spot on — and I like that lipsum_words_from_seed can reuse the thread-local LOREM_IPSUM_CHAIN variable this way.

Would you be up for restructuring things as you suggest?

@Diggsey
Copy link
Contributor Author

Diggsey commented May 30, 2021

Yeah, it will be a breaking change though.

@mgeisler
Copy link
Owner

Yeah, it will be a breaking change though.

That's okay, especially since the lipsum_* functions will stay unchanged, right?

The few dependencies of the crate seem to use those high-level functions, so they should have no problem upgrading.

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 a pull request may close this issue.

2 participants