Skip to content
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

doc: fixed variable name issue & added header files #2150

Merged
merged 1 commit into from Jan 15, 2020

Conversation

@Hemal-Mamtora
Copy link
Contributor

Hemal-Mamtora commented Jan 12, 2020

This PR has 2 changes, which might not be apparent due to a single commit. The changes are as follows:

  1. Updated a variable name : trainLabelsTemp to trainLabels. Otherwise the example would not compile giving a undeclared Variable Error. [This change was necessary]

  2. Added Header files, namespace and main(), so that the code is an easy-run copy-paste kind. [This is a suggested Change]


// Split the data from the training set.
arma::mat trainLabelsTemp = dataset.submat(dataset.n_rows - 3, 0,

This comment has been minimized.

Copy link
@Hemal-Mamtora

Hemal-Mamtora Jan 12, 2020

Author Contributor

Check deleted(red) line number 218 and 207 to understand the first change.
207: - arma::mat trainLabelsTemp = dataset.submat(dataset.n_rows - 3, 0, ...
218: - model.Train(trainData, trainLabels);
//I should have made 2 commits, that would have made it easy to spot.

This comment has been minimized.

Copy link
@zoq

zoq Jan 13, 2020

Member

Most of the time I look at the diff anyway, but this becomes definitely helpful if you take a look at changes at some point in the review process.

@Hemal-Mamtora Hemal-Mamtora changed the title fixed variable name issue & added header files doc: fixed variable name issue & added header files Jan 12, 2020
@zoq
zoq approved these changes Jan 13, 2020
Copy link
Member

zoq left a comment

Awesome, thanks!


// Split the data from the training set.
arma::mat trainLabelsTemp = dataset.submat(dataset.n_rows - 3, 0,

This comment has been minimized.

Copy link
@zoq

zoq Jan 13, 2020

Member

Most of the time I look at the diff anyway, but this becomes definitely helpful if you take a look at changes at some point in the review process.

@mlpack-bot
mlpack-bot bot approved these changes Jan 14, 2020
Copy link

mlpack-bot bot left a comment

Second approval provided automatically after 24 hours. 👍

@zoq zoq merged commit 4051e29 into mlpack:master Jan 15, 2020
18 checks passed
18 checks passed
LaTeX Documentation Checks Build finished.
Details
Memory Checks Build finished.
Details
Static Code Analysis Checks Build finished.
Details
Style Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
mlpack.mlpack Build #20200112.4 succeeded
Details
mlpack.mlpack (Linux Julia) Linux Julia succeeded
Details
mlpack.mlpack (Linux Markdown) Linux Markdown succeeded
Details
mlpack.mlpack (Linux Plain) Linux Plain succeeded
Details
mlpack.mlpack (Linux Python27) Linux Python27 succeeded
Details
mlpack.mlpack (Linux Python37) Linux Python37 succeeded
Details
mlpack.mlpack (Windows VS14 Plain) Windows VS14 Plain succeeded
Details
mlpack.mlpack (Windows VS15 Plain) Windows VS15 Plain succeeded
Details
mlpack.mlpack (Windows VS16 Plain) Windows VS16 Plain succeeded
Details
mlpack.mlpack (macOS Julia) macOS Julia succeeded
Details
mlpack.mlpack (macOS Plain) macOS Plain succeeded
Details
mlpack.mlpack (macOS Python27) macOS Python27 succeeded
Details
mlpack.mlpack (macOS Python37) macOS Python37 succeeded
Details
@zoq

This comment has been minimized.

Copy link
Member

zoq commented Jan 15, 2020

Thanks for the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.