Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Mar 26, 2020

What changes were proposed in this pull request?

Add end-to-end training word vector and use the trained word vector for classification as mentioned in issues #877

How was this patch tested?

None

Please review
https://github.com/eclipse/deeplearning4j/blob/master/CONTRIBUTING.md before opening a pull request.

Signed-off-by: William Ardianto <[email protected]>
@saudet saudet requested a review from treo March 26, 2020 10:15
Copy link
Member

@treo treo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for creating an example for something that is requested. I have a few change requests:

  • We usually tell people to use a record reader instead of creating their own data set iterator, so in this case I'd like to also see a record reader being used instead of you creating yet another data set iterator. I know this is different from what we do in other examples here, but for new examples we'd like it to follow our guidelines better than what the existing examples do.

  • We want to make new examples better than the one that already exist. This means that every new example should have a javadoc at the beginning which tells the user what they are going to learn from this example and how it is different from other examples. Also, there should be comments within the example itself which highlight the specific region where the example is doing something different.

  • And finally it would also be nice if you can add to the description how long the example is expected to run and how much resources it requires. Especially for examples that train Word2Vec it often takes quite a long time to run them. So people should be able to decide up front whether they want to actually run the example or if they want to just read the source code to learn from it.

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.

2 participants