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

random: add --time option, update docs #2322

Merged
merged 2 commits into from Dec 26, 2016

Conversation

diomekes
Copy link

Adds feature #2305

@diomekes
Copy link
Author

I just realized that this does not work with -a. I need to figure out how to list the individual tracks of an album in order to get the total length.

@sampsyo
Copy link
Member

sampsyo commented Dec 17, 2016

Ah, tricky!

Someone was asking on IRC about adding a pseudo-field to calculate exactly that. But in the mean time, you can use something like sum(a.length for a in album.tracks()) to get the total time for an album.

@diomekes
Copy link
Author

diomekes commented Dec 17, 2016

It's pretty slow with -a, maybe someone knows how to optimize it. Otherwise, it seems it all works.

Oh, in case someone comes across this randomly, it's album.items(), and not .tracks(), as written in the above post.

@bearcatsandor
Copy link

It seems fast enough for me with -a. I love it! Are there still plans to integrate it with the 'beet play' command?

@diomekes
Copy link
Author

I have #2304 open that adds --random to the play plugin, but if this gets merged, I'll close that one and open a new one because I had made some changes that didn't need to be made.

@bearcatsandor
Copy link

awesomeness.

@diomekes
Copy link
Author

Are there any other steps I need to take in order to get this ready to be merged?

@sampsyo sampsyo merged commit 1bc5456 into beetbox:master Dec 26, 2016
sampsyo added a commit that referenced this pull request Dec 26, 2016
random: add --time option, update docs
sampsyo added a commit that referenced this pull request Dec 26, 2016
sampsyo added a commit that referenced this pull request Dec 26, 2016
sampsyo added a commit that referenced this pull request Dec 26, 2016
Also, avoid setting an unnecessary field on the album objects. :/
sampsyo added a commit that referenced this pull request Dec 26, 2016
sampsyo added a commit that referenced this pull request Dec 26, 2016
This is the payoff from the earlier refactorings: the control flow is now
consistent and clear, and the two factors (time vs. number, equal-chance or
not) are orthogonal. See also #2322.
@sampsyo
Copy link
Member

sampsyo commented Dec 26, 2016

Nope; I was the bottleneck here, sorry! I've merged, and I've also done some substantial refactoring. Can you please take a look to check that I didn't mess anything up too badly?

Also, one thing came up when I was looking over the code: I noticed that your strategy will often include the shortest songs in a person's library. That is, if I have a 2-second song somewhere, then it's quite common that there will be a 2-second shortfall when selecting other songs, so that short song is very likely to be picked at the end.

I don't have any great ideas for how to resolve that, but I thought I'd mention it.

@diomekes
Copy link
Author

Those changes to the code look great, thanks for taking the time to clean it up. My simple tests show everything is all good.

As far as the short song issue, I though of that too, but couldn't think of any way around it. A user-based solution might be to use queries like length to filter out a known short song that keeps being used, but with a big library there usually are several short songs that would fill the gap, and maybe not be repeated as often.

@ArchangeGabriel
Copy link

Another option could be to have an option of a minimum duration for a song to be considered. For instance, I might not want any song less than 1 min to be added to the playlists: I have no way to check this right now, but I, for one, expect all such songs of my collection to have no interest out of context. ;)

@sampsyo
Copy link
Member

sampsyo commented Dec 27, 2016

I like the minimum-length heuristic. It might be worth noting that the problem of finding a playlist of music that comes closest to a given maximum time (without going over) is exactly the bin packing problem, which is NP-complete. That implies that it might be tricky to get a great solution here, although the addition of randomness makes the question a little more complicated.

@diomekes
Copy link
Author

Can a minimum length not be specified with 'length:0:02..' to get tracks longer than 2 seconds? That's what I was suggesting earlier, but I've never used that query...

@diomekes diomekes deleted the random-timelimit branch December 28, 2016 01:49
@sampsyo
Copy link
Member

sampsyo commented Dec 28, 2016

Yes, good point—that query does work out of the box for avoiding short songs, @diomekes.

I've also spent a little while thinking about the algorithmic problem (randomly sampling a bin packing solution) and I still don't see a clearly better solution than the current greedy algorithm. Maybe one small improvement, however, would be to shuffle the resulting list before returning it. This way, at least the short tracks won't always appear at the end of the playlist?

@diomekes
Copy link
Author

Yes, the list could be shuffled a second time after going through the timed selection process.

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

4 participants