Feat/sort FSRSItem by length to speed up training

[L-M-Sherlock]

Note: this code remove shuffle of items. It would break stochastic gradient descent. I wander whether it is possible to shuffle at the level of batchs rather than items.

[dae]

Is the change I pushed what you had in mind? For a given review length, the items should now appear in a random order.

[L-M-Sherlock]

It's effect is nuanced to the training. Because the order of item in the same batch is not important. I plan to implement a shuffle in batch-level.

https://github.com/burn-rs/burn/blob/main/burn-core/src/data/dataloader/strategy.rs

[dae]

So you want a batch of size 3, then a batch of size 10, then a batch of size 2, etc?

[L-M-Sherlock]

So you want a batch of size 3, then a batch of size 10, then a batch of size 2, etc?

I want a batch with seq_len = 3, then a batch with seq_len = 10, then a batch with seq_len = 2.

[dae]

Ok, how about now? A test is currently failing due to #31, as I rebased this PR on the main branch.

[L-M-Sherlock]

I have seen your code. But I think we should write a new dataloader rather than manipulate the dataset before build the dataloader. I'm coding now.

[L-M-Sherlock]

The shuffle only applies once in your implementation (if I'm wrong, please feel free to correct me). Ideally, the shuffle should apply before each epoch of training.

[dae]

Ok, I see. Hopefully the code will be useful as a reference anyway. :-)

[L-M-Sherlock]

@dae, I have a weird observation during my coding. The results of training always vary over time, even when I remove shuffle. Then I figure it out. It's caused by grouped.into_values() in the group_by_cid(). It's unstable in order. So I apply sorted_by_cached_key(|revlog| revlog.get(0).unwrap().cid) for that. I'm not sure that it's the best practice, but it works.

Then, I implement BatchShuffleDataset, which could Shuffle the indices by batch_size. But I'm not sure whether the training loop would shuffle the dataset before each epoch. I need to ask @nathanielsimard.

[L-M-Sherlock]

Wait for tracel-ai/burn#703

[dae]

In most languages, HashMaps don't retain the order items are put into them - Python's dictionaries are a bit special in that regard. I think we can do this a bit more efficiently by sorting on the SQL end and then using .group_by() - I've pushed an update and some other tidyups.

[dae]

Nice work on the batch shuffling! And thanks @nathanielsimard for the upstream fix.

[dae]

Would you like me to rebase this or would you like to?

[L-M-Sherlock]

Would you mind doing it? Thanks. I'm out right now.

[dae]

If you guys are happy with the current state of this, I'd suggest we merge it in, even though the PR mentioned above hasn't landed yet.

[L-M-Sherlock]

Should we update the dependencies of burn in Cargo.toml before merging?

[L-M-Sherlock]

Ignore my last comment. I confused this RR as another PR.