Skip to content

Conversation

@JRAlexander
Copy link
Contributor

@JRAlexander JRAlexander commented Apr 19, 2019

Extended SentimentPrediction for easier display of results
Internal Review URL

Related code sample: dotnet/samples#832

Copy link
Contributor

@luisquintanilla luisquintanilla left a comment

Choose a reason for hiding this comment

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

Nice job. Minor changes

[!code-csharp[BuildTuples](~/samples/machine-learning/tutorials/SentimentAnalysis/Program.cs#BuildSentimentPredictionPairs "Build the pairs of sentiment data and predictions")]

Now that `SentimentText` and `Sentiment` are combined in a class, display the results:
Because `SentimentPrediction` inherited from `SentimentData`, the `Transform()` method populated `SentimentText` with the predicted fields. As the ML.NET process processes, each component adds columns, and this makes it easy to display the results:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion:

"Because SentimentPrediction is inherited from SentimentData, the Transform() method populated SentimentText with the values in the SentimentText column. Throughout the ML.NET model building process, each component adds columns, and this makes it easy to display the result"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent! Thanks, @luisquintanilla!

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear how the concepts in the two sentences connect here. What does the user need to know? That the fields in SentimentPrediction will be in the transformed result. The inheritance is an implementation detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the fields need to be there in order for the results to appear. Modified. Any clearer?

Copy link
Contributor

@natke natke left a comment

Choose a reason for hiding this comment

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

Looks good. A couple of comments.

|Service was very prompt. | 1 |

`SentimentPrediction` is the prediction class used after the model training. It has a single boolean (`Sentiment`) and a `PredictedLabel` `ColumnName` attribute. The `Label` is used to create and train the model, and it's also used with the split out test dataset to evaluate the model. The `PredictedLabel` is used during prediction and evaluation. For evaluation, training data, the predicted values, and the model are used.
`SentimentPrediction` is the prediction class used after the model training. It inherits from `SentimentData` for displaying the `SentimentText` with the predictions. `SentimentPrediction` has a single boolean (`Sentiment`) and a `PredictedLabel` `ColumnName` attribute. The `Label` is used to create and train the model, and it's also used with the split out test dataset to evaluate the model. The `PredictedLabel` is used during prediction and evaluation. For evaluation, training data, the predicted values, and the model are used.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if a simple diagram (stylized UML) would help here? Even if we don't add a diagram, I think we can simplify the text. The fields are already in the code, so we don't need to repeat that here, but rather focus on the information that helps to interpret the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, and this is really a comment for the sample, would it simplify this explanation to not re-map the column names. i.e.

Use Label for the training label; and PredictedLabel for the prediction?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified.

[!code-csharp[BuildTuples](~/samples/machine-learning/tutorials/SentimentAnalysis/Program.cs#BuildSentimentPredictionPairs "Build the pairs of sentiment data and predictions")]

Now that `SentimentText` and `Sentiment` are combined in a class, display the results:
Because `SentimentPrediction` inherited from `SentimentData`, the `Transform()` method populated `SentimentText` with the predicted fields. As the ML.NET process processes, each component adds columns, and this makes it easy to display the results:
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear how the concepts in the two sentences connect here. What does the user need to know? That the fields in SentimentPrediction will be in the transformed result. The inheritance is an implementation detail.

@JRAlexander
Copy link
Contributor Author

@luisquintanilla - I believe I've addressed your changes.

@JRAlexander JRAlexander merged commit fb638a8 into dotnet:master Apr 23, 2019
@JRAlexander
Copy link
Contributor Author

Thanks, @luisquintanilla and @natke for the thoughtful feedback and time spent. Much appreciated!

@JRAlexander JRAlexander deleted the jralexander-041919-01 branch April 24, 2019 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants