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

Option inconsistency in perceptron #1882

Merged
merged 10 commits into from May 12, 2019

Conversation

Projects
None yet
3 participants
@jeffin143
Copy link
Contributor

commented Apr 26, 2019

relevant #1565

@jeffin143

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

Now, The code looks ok to me, but static Code Analysis fails, Saying that p is used after deleting .

jeffin143 added some commits Apr 27, 2019

@zoq

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

In this case the static code analysis result is false, so we can ignore the issue.

@zoq zoq added c: documentation and removed s: unlabeled labels Apr 27, 2019

@rcurtin
Copy link
Member

left a comment

Hey @jeffin143, thanks for putting time into this. I think it is looking good and all my comments are very minor. If you can handle them then from my side it is ready for merge. Do you want to also update HISTORY.md with these changes?

jeffin143 added some commits May 9, 2019

Show resolved Hide resolved HISTORY.md Outdated
Show resolved Hide resolved src/mlpack/methods/adaboost/adaboost_main.cpp Outdated
Show resolved Hide resolved src/mlpack/methods/adaboost/adaboost_main.cpp Outdated
Show resolved Hide resolved src/mlpack/methods/adaboost/adaboost_main.cpp Outdated
Show resolved Hide resolved src/mlpack/methods/perceptron/perceptron_main.cpp Outdated
Show resolved Hide resolved src/mlpack/methods/perceptron/perceptron_main.cpp Outdated
@rcurtin
Copy link
Member

left a comment

Looking good, but I realize there's one thing I forgot---could you add a really quick test to main_tests/perceptron_test.cpp to make sure the output and predictions outputs are the same? Here's some code that should do it (I think):

// This test can be removed in mlpack 4.0.0.
BOOST_AUTO_TEST_CASE(PerceptronOutputPredictionsCheck)
{
  arma::mat trainX1;
  arma::Row<size_t> labelsX1;

  // Loading a train data set with 3 classes.
  if (!data::Load("vc2.csv", trainX1))
  {
    BOOST_FAIL("Could not load the train data (vc2.csv)");
  }

  // Loading the corresponding labels to the dataset.
  if (!data::Load("vc2_labels.txt", labelsX1))
  {
    BOOST_FAIL("Could not load the train data (vc2_labels.csv)");
  }

  SetInputParam("training", std::move(trainX1)); // Training data.
  // Labels for the training data.
  SetInputParam("labels", std::move(labelsX1));

  // Training model using first training dataset.
  mlpackMain();

  // Check that the outputs are the same.
  CheckMatrices(CLI::GetParam<arma::Row<size_t>>("output"),
                CLI::GetParam<arma::Row<size_t>>("predictions"));
}

For AdaBoost it should be easy too:

BOOST_AUTO_TEST_CASE(AdaBoostOutputPredictionsTest)
{
  arma::mat trainData;
  if (!data::Load("vc2.csv", trainData))
    BOOST_FAIL("Unable to load train dataset vc2.csv!");

  arma::Row<size_t> labels;
  if (!data::Load("vc2_labels.txt", labels))
    BOOST_FAIL("Unable to load label dataset vc2_labels.txt!");

  SetInputParam("training", std::move(trainData));
  SetInputParam("labels", std::move(labels));

  mlpackMain();

  CheckMatrices(CLI::GetParam<arma::Row<size_t>>("output"),
                CLI::GetParam<arma::Row<size_t>>("predictions"));
}

Anyway, I think those should work...

jeffin143 added some commits May 11, 2019

@jeffin143

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2019

Looking good, but I realize there's one thing I forgot---could you add a really quick test to main_tests/perceptron_test.cpp to make sure the output and predictions outputs are the same? Here's some code that should do it (I think):

// This test can be removed in mlpack 4.0.0.
BOOST_AUTO_TEST_CASE(PerceptronOutputPredictionsCheck)
{
  arma::mat trainX1;
  arma::Row<size_t> labelsX1;

  // Loading a train data set with 3 classes.
  if (!data::Load("vc2.csv", trainX1))
  {
    BOOST_FAIL("Could not load the train data (vc2.csv)");
  }

  // Loading the corresponding labels to the dataset.
  if (!data::Load("vc2_labels.txt", labelsX1))
  {
    BOOST_FAIL("Could not load the train data (vc2_labels.csv)");
  }

  SetInputParam("training", std::move(trainX1)); // Training data.
  // Labels for the training data.
  SetInputParam("labels", std::move(labelsX1));

  // Training model using first training dataset.
  mlpackMain();

  // Check that the outputs are the same.
  CheckMatrices(CLI::GetParam<arma::Row<size_t>>("output"),
                CLI::GetParam<arma::Row<size_t>>("predictions"));
}

For AdaBoost it should be easy too:

BOOST_AUTO_TEST_CASE(AdaBoostOutputPredictionsTest)
{
  arma::mat trainData;
  if (!data::Load("vc2.csv", trainData))
    BOOST_FAIL("Unable to load train dataset vc2.csv!");

  arma::Row<size_t> labels;
  if (!data::Load("vc2_labels.txt", labels))
    BOOST_FAIL("Unable to load label dataset vc2_labels.txt!");

  SetInputParam("training", std::move(trainData));
  SetInputParam("labels", std::move(labels));

  mlpackMain();

  CheckMatrices(CLI::GetParam<arma::Row<size_t>>("output"),
                CLI::GetParam<arma::Row<size_t>>("predictions"));
}

Anyway, I think those should work...

@rcurtin , Read your message on IRC about reviewing 3 PRs a day , I know it could be hectic , I would be more cautious about the style error i make so that there could be less load on you.

Here is the Screen Shot , just to make review easy :)
The following two are for the adaboost 👍
Screenshot from 2019-05-11 07-35-58
Screenshot from 2019-05-11 07-34-52

The following two are for the Perceptron :
Screenshot from 2019-05-11 07-34-39
Screenshot from 2019-05-11 07-33-52

@zoq

zoq approved these changes May 11, 2019

Copy link
Member

left a comment

Looks good to me, no further comments from my side.

@rcurtin rcurtin merged commit 06efecb into mlpack:master May 12, 2019

6 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
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rcurtin

This comment has been minimized.

Copy link
Member

commented May 12, 2019

I would be more cautious about the style error i make so that there could be less load on you.

Thanks, I appreciate that. It's no huge issue on my end to point things out though. :)

Thanks again for the fix. I made some other quick style changes in 328dd57. 👍

@jeffin143 jeffin143 deleted the jeffin143:option-inconsistency branch May 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.