Sentiment analysis pytorch

[owoshch]

No description provided.

[Maerville]

in def train i would recommend to set default values as well

what's the magic number 25000?

[Maerville]

what's the point in making ConvNet with customizable Emb layer size if this parameter is not accessible via command line args?

[Maerville]

Is it better to separate the script for two files: one for the network and utils and second for main?

as for now you are using this network only in one file, I would recommend to leave it as it is. it makes sense to put NN to separate file once you use it in at least 2-3 different workflows :)

[Maerville]

Also pls remove unnecessary blank lines

[Maerville]

How should I white docstrings?

Well, in this particular file I'd say def train is worth documenting params. Other functions are pretty much understandable even without excessive comments
http://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html

[xyhuang]

Please see inline comments, also please check out the style guide for some styling conventions,
e.g.
https://www.python.org/dev/peps/pep-0008/#imports
https://www.python.org/dev/peps/pep-0008/#function-and-variable-names
https://www.python.org/dev/peps/pep-0008/#indentation
https://www.python.org/dev/peps/pep-0008/#blank-lines
https://www.python.org/dev/peps/pep-0257/
etc.
ideally you may want to run it through pylint or pep8 to fix styling issues but the aboves are what i spotted.

[xyhuang]

do we need this?

[xyhuang]

plotting will be a performance hit to the training, i don't think it is desirable in benchmark... correct me if i'm wrong.

[ddutta]

+1 .... avoid plotting and print statements please :)

…

On Tue, Jun 12, 2018 at 2:45 PM Xinyuan Huang ***@***.***> wrote: ***@***.**** requested changes on this pull request. Please see inline comments, also please check out the style guide for some styling conventions, e.g. https://www.python.org/dev/peps/pep-0008/#imports https://www.python.org/dev/peps/pep-0008/#function-and-variable-names https://www.python.org/dev/peps/pep-0008/#indentation https://www.python.org/dev/peps/pep-0008/#blank-lines https://www.python.org/dev/peps/pep-0257/ etc. ideally you may want to run it through pylint or pep8 to fix styling issues but the aboves are what i spotted. ------------------------------ In sentiment_analysis/pytorch/train.py <#1 (comment)>: > + model.eval() + for batch in tqdm.tqdm(test_iter): + data = Variable(batch.text) + target = Variable(batch.label) - 1 + output = model(data) + loss = F.nll_loss(output, target) + pred = torch.max(output, 1)[1].data.numpy() + acc = np.mean(pred == target.data.numpy()) + #if acc >= quality: + # break + acc_log.append(acc) + loss = loss.data[0] + loss_log.append(loss) + return loss_log, acc_log + +def plot_history(train_history, val_history, title='loss'): do we need this? ------------------------------ In sentiment_analysis/pytorch/train.py <#1 (comment)>: > + train_log, train_acc_log = [], [] + val_log, val_acc_log = [], [] + + for epoch in range(n_epochs): + train_loss, train_acc = train_epoch(model, optimizer, train_iter) + + val_loss, val_acc = test(model, test_iter, quality) + + train_log.extend(train_loss) + train_acc_log.extend(train_acc) + + steps = 25000 / batch_size + val_log.append((steps * (epoch + 1), np.mean(val_loss))) + val_acc_log.append((steps * (epoch + 1), np.mean(val_acc))) + + plot_history(train_log, val_log) plotting will be a performance hit to the training, i don't think it is desirable in benchmark... correct me if i'm wrong. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA1h-ukZG-uHHu31LK-CmqoQ-yVoQXlAks5t8DZmgaJpZM4UdCk1> .

-- -Debo~

[ElizabethSvit]

did you test it on gpu?

[ElizabethSvit]

just tqdm(train_iter)?