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

more efficient implementation of runMarkov #1068 #1179

Closed
wants to merge 3 commits into from

Conversation

jwaldmann
Copy link
Contributor

No description provided.

@sss-create sss-create requested a review from yaxu March 4, 2025 11:18
@sss-create
Copy link
Collaborator

I am not sure how we feel about adding new dependencies. I requested a review from Alex..

@yaxu
Copy link
Member

yaxu commented Mar 4, 2025

Yes would like to avoid new dependencies if possible

@jwaldmann
Copy link
Contributor Author

jwaldmann commented Mar 5, 2025

performance of accessing a sequence by index (inside runMarkov):

  • current implementation: linked list O(n),
  • this PR: vector (new dependency) O(1)
  • another option: containers.Data.Map (existing dependency) O(log n)

It feels wrong to have a contiguous range of Int as keys, and not use the most efficient data structure for that.

What exactly is the problem with adding vector? Did tidal have trouble with too much dependencies in the past? Perhaps my implementation could be rewritten to use array, but that's also a fresh dependency, albeit a trivial one (I guess it comes sort-of pre-installed with ghc but I don't know current status.) Or use containers.Data.IntMap as a work-around (which still feels wrong).

On the other hand, one might ask, what's the problem with inefficient runMarkov? Maybe it's not that important - until we want to use a large state space. Do we? Then it's large-scale programmed music, and no longer live coding?

@jwaldmann jwaldmann closed this Mar 7, 2025
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.

3 participants