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

Optional PR #152

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Optional PR #152

wants to merge 4 commits into from

Conversation

asigalov61
Copy link

I think replacing regular random with secrets.random should improve the generating pattern/uniqueness of the pattern.

These are just my humble suggestions and it is totally up to you guys... :)

@coveralls
Copy link

coveralls commented Jan 28, 2021

Coverage Status

Coverage remained the same at 100.0% when pulling bcf42a3 on asigalov61:master into c8eb4cf on jsvine:master.

@asigalov61
Copy link
Author

@jsvine Per your suggestion I added my markovify Piano to the list of implementations/examples.

Thanks.

@jsvine
Copy link
Owner

jsvine commented Feb 4, 2021

Thanks for the suggestion, @asigalov61. Could you explain more about (a) the motivation for using secrets, and (b) the particular implementation here?

As far as I can tell, the standard random module seems like a good fit for this library. Per the Python documentation:

In particular, secrets should be used in preference to the default pseudo-random number generator in the random module, which is designed for modelling and simulation, not security or cryptography.

... but markovify lands pretty directly in the "modelling and simulation" realm. What benefit would secrets provide here, and what sort of performance increase/decrease would it cause?

And on your piano example: Really fun, thanks! Could you, however, submit that as a separate PR?

@asigalov61
Copy link
Author

@jsvine Thank you for complimenting my work. It means a lot to me. I will submit a separate PR with the link. Thank you.

As far as the secrets module goes... while the standard random module is decent and fits fine with your implementation, it is always a good idea to use a more random and more secure version of the random generator. Not only it is a best practice but it would also help to reduce Markov plagiarism and repetitions as it can do it under certain circumstances.

In general, using the secrets random should produce better results (both for text and music from my direct experience).
I modified my copy with secrets.random just like my PR and it works slightly better IMHO.

Otherwise, you are correct, and the standard random module is probably fine to use too. Although if I were you I would advise the users about it and let them know about the plagiarism/repetitions possibilities.

As a note, in AI music, plagiarism and repetitions are a huge issue and this is where secrets.random helps, so I make my suggestion from this particular perspective. So using secrets may not be critical/needed for other purposes.

I hope this makes sense.

Thanks.

@jsvine
Copy link
Owner

jsvine commented Feb 19, 2021

I will submit a separate PR with the link.

Great, thanks!

As far as the secrets module goes... while the standard random module is decent and fits fine with your implementation, it is always a good idea to use a more random and more secure version of the random generator. Not only it is a best practice but it would also help to reduce Markov plagiarism and repetitions as it can do it under certain circumstances.

As I understand it, random vs. secrets should have no effect on randomness at a human-perceptual level, nor on Markov repetitions. As the secrets module states, random is "designed for modelling and simulation," which is Markovify's use-case. As I understand it, what secrets provides is enhanced security (e.g., by making it much harder to guess outputs) — something not needed by Markovify.

That said, I'm open to being convinced otherwise. If you or someone else can point me to documentation that demonstrates secrets's superiority for simulation, that'd be great. Alternatively, perhaps you could provide a small script that demonstrates a difference in repetitions.

@asigalov61
Copy link
Author

@jsvine Apologies for the delayed response. I sorta missed your last response...

Yes, I agree that difference between secrets and standard random is not that big perceptually so I am not saying that it will offer a significant improvement.

However, from my understanding, secrets.random uses crypto algos + oher tricks to not only make it as random as possible, but also it provides a much better random distribution, which is quite similar to how AI models do it, which is why I suggested looking into it.

I will try to make an example for you if I can to show how and where it is different, but please give me some time as it is not a priority issue for both of us.

And thank you very much again for hearing my suggestions and for making this awesome markov-chain implementation. Much much appreciated.

Alex.

PS. I was even able to generate multi-instrumental music with markovify. Check it out here if you are curious. Thanks.
https://github.com/asigalov61/Markovify-Piano/tree/main/Output_Samples/Multi-Instrumental

@asigalov61
Copy link
Author

@jsvine Btw, I wanted to kindly ask you to add my Markovify Piano to your list, please because for the love of God I can't figure out how to make a separate pull request for README. I still can't figure out GitHub PRs so your help will be much appreciated.

Or, if you can tell me how to do it, it would be great too but I think it is just easier to add manually IMHO.

Thank you.

Here is the code:

[Markovify Piano](https://github.com/asigalov61/Markovify-Piano) Coherent and plausible music generation with Markov-chain/model.

@jsvine
Copy link
Owner

jsvine commented Mar 26, 2021

Btw, I wanted to kindly ask you to add my Markovify Piano to your list

No problem, now done: b0bfd2a

@asigalov61
Copy link
Author

@jsvine Thank you very much for your help and for the great software. If you need any help or want to share something, do not hesitate to contact me. Would be happy to help in return.

Alex

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.

None yet

3 participants