Skip to content

ML.NET tutorials - bug fixes #5333

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
May 23, 2018
Merged

ML.NET tutorials - bug fixes #5333

merged 11 commits into from
May 23, 2018

Conversation

mairaw
Copy link
Contributor

@mairaw mairaw commented May 12, 2018

Fixes #5330
Fixes #5329
Fixes #5343
Fixes #5366
Fixes #5346

Companion PR: dotnet/samples#70

Main things addressed in this PR are:

@mairaw mairaw requested review from asthana86 and aditidugar-zz May 12, 2018 04:44
Copy link
Contributor

@rpetrusha rpetrusha left a comment

Choose a reason for hiding this comment

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

LGTM, @mairaw. This is ready to merge.


In Solution Explorer, right-click on your project and select **Manage NuGet Packages**. Choose "nuget.org" as the Package source, select the Browse tab, search for **Microsoft ML.NET**, select that package in the list, and select the **Install** button. If prompted to select a package management format, select **PackageReference in project file**.
In Solution Explorer, right-click on your project and select **Manage NuGet Packages**. Choose "nuget.org" as the Package source, select the Browse tab, search for **Microsoft.ML**, select that package in the list, and select the **Install** button. Select the **OK** button on the **Preview Changes** dialog and then select the **I Accept** button on the **License Acceptance** dialog if you agree with the license terms for the packages listed.
Copy link
Contributor

Choose a reason for hiding this comment

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

@mairaw it looks that using PackageReference is essential: dotnet/machinelearning#93

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pkulikov Humm, if I follow the steps and add the reference, the package reference is added automatically for me. And even though my settings for NuGet are set as packages.config in VS, it correctly added to csproj file as expected. @eerhardt do you have more context on that issue Petr referenced above? How am I getting PackageReference by default? Is it because it's a .NET Core project?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, if you are using a .NET Core project, or any "SDK-based" project (<Project Sdk="Microsoft.NET.Sdk"> at the top of the .csproj), you get PackageReference by default. For classic .csproj files, you can set the default in VS Tools => Options => NuGet Package Manager => General

image

I'm working on a fix for dotnet/machinelearning#93, so that should be fixed in the next release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for confirming @eerhardt. So it's fine to remove that step since the tutorial is using a .NET Core app.

@@ -273,4 +269,4 @@ In this tutorial, you learned how to:

Check out our GitHub repository to continue learning and find more samples.
> [!div class="nextstepaction"]
> [GitHub Repository](https://github.com/dotnet/machinelearning/)
> [dotnet/machilearning GitHub repository](https://github.com/dotnet/machinelearning/)
Copy link
Member

Choose a reason for hiding this comment

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

type-o dotnet/machilearning => dotnet/machinelearning

@mairaw mairaw added the WIP label May 16, 2018
@mairaw mairaw changed the title NuGet, source code and other edits WIP NuGet, source code and other edits May 16, 2018
@mairaw mairaw changed the title WIP NuGet, source code and other edits WIP ML.NET tutorials - bug fixes May 17, 2018
@mairaw mairaw added the 🚧 Hold for related PR Indicates a PR can only be merged when other related PRs are merged (see comments for links) label May 17, 2018
@@ -110,35 +114,35 @@ public static PredictionModel<TaxiTrip, TaxiTripFarePrediction> Train()

## Create a learning pipeline

Now create the learning pipeline. The learning pipeline loads all of the data and algorithms necessary to train the model. Copy the following code into the `Train()` method:
The learning pipeline loads all of the data and algorithms necessary to train the model. Add the following code into the `Train()` method:
Copy link
Contributor

Choose a reason for hiding this comment

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

@mairaw nit and question: methods should be mentioned without parenthesis?
So, not Train() but Train in the second sentence?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both are ok but I changed to the version without the parentheses to match how the other tutorial was using it.


[!code-csharp[UsingTasks](../../../samples/machine-learning/tutorials/TaxiFarePrediction/Program.cs#9 "Add System.Threading.Tasks. to your usings.")]

Because the `async Main` method is a new feature in C# 7.1 and the default language version of the project is C# 7, you need to change the language version to C# 7.1 or higher.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: C# 7 -> C# 7.0

@mairaw mairaw requested a review from JRAlexander as a code owner May 22, 2018 01:44
@mairaw mairaw removed the WIP label May 22, 2018
@mairaw mairaw changed the title WIP ML.NET tutorials - bug fixes ML.NET tutorials - bug fixes May 22, 2018
@mairaw
Copy link
Contributor Author

mairaw commented May 22, 2018

@JRAlexander I think this is ready for a final review. We need the samples changes merged for this PR to pass.

@mairaw mairaw added the P0 label May 22, 2018
@JRAlexander
Copy link
Contributor

Closing and reopening to restart failed build

@JRAlexander JRAlexander reopened this May 23, 2018
@mairaw mairaw closed this May 23, 2018
@mairaw mairaw reopened this May 23, 2018
Copy link
Contributor

@JRAlexander JRAlexander left a comment

Choose a reason for hiding this comment

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

LGTM.

@mairaw mairaw closed this May 23, 2018
@mairaw mairaw reopened this May 23, 2018
@mairaw mairaw merged commit d2fad55 into dotnet:master May 23, 2018
@mairaw mairaw deleted the ml.net branch May 23, 2018 16:53
@mairaw mairaw removed the 🚧 Hold for related PR Indicates a PR can only be merged when other related PRs are merged (see comments for links) label Nov 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants