mlverse/ISLR-deeplearning-torch

Ready for review

Closed this issue · 4 comments

Hey @skeydan @jonathanbratt @jonthegeek @dcdorukcengiz

I mostly finished porting the Deep Learning chapter from Keras to luz/torch.
I wonder if you could review the text in this repository.

Currently it requires dev version of all packages: torch, luz, torchvision, torchdatasets. but we will publish them to CRAN once the review is finished.

There's a rendered version available at: https://rpubs.com/dfalbel/815416

Best,

Hi @dfalbel , this looks awesome! Just submitted a PR for minor stuff.

Here are a few additional comments & questions that didn't fit there, -

I guess we'll leave it to the ISLR authors how they'd like to present the torch version on their website, but for now, I've removed this:

This code is impressively fast, and the package is well-structured. A good companion is the text Deep Learning with R (F. Chollet and J.J. Allaire, (2018), Manning Publications,) and most of our code is adapted from there.

In the text it says that on the validation set, we're using MAE, but the code does not ... I guess one of those needs to be changed ...

Should we use more descriptive variable names for the models that are created? Maybe also, using underscores?

For IMDB, we're following the Keras example code, right? It's a bit of a pity I think, because it gets so much harder with all the one-hot encoding ... But it would of course be weird to have one example using embeddings, and the other, not. (Ah okay, I now see that the second IMDB example uses embeddings. Then probably the idea is to show both ways.)

For the RNN, in torch we would probably not create the input data that way, right? I would assume a more idiomatic way should be via subclassing imdb_dataset()? If that's correct, should we show that?

As this is a new repo I was expecting the main branch to be, well, main :-))
Could you please check your .gitconfig?
(should be

[init]
	defaultBranch = main

Thanks!

I think all comments are addressed besides 5. I think so, but I also think the idea here is to show the pre-processing steps too. In the following example we show how todo same thing using the imd_dataset.

Also renamed the branch to main.

FYI my book club isn't to this chapter yet. I think it looks great but I might have more thoughts in a few weeks.

Cool! Thanks @jonthegeek !
I'm closing this because I just sent this to ISLR authors and it's about to go to their website.