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

iterative machine unit test #4370

Merged

Conversation

shubham808
Copy link
Contributor

Tests that train 3 models:

  1. Complete train till convergence
  2. Train with max number of iteration set low at x
  3. Train a model then prematurely stop it at x iterations, the continue training till convergence

The results are compared to ensure proper state update in every iteration.
Eventually we would want this done automatically for all Iterative Machines.

auto svm_one = some<CNewtonSVM>();
svm_one->set_labels(labels);

// trained only till 5 iteraetions
Copy link
Member

Choose a reason for hiding this comment

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

typo

* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are met:
*
* 1. Redistributions of source code must retain the above copyright notice, this
Copy link
Member

Choose a reason for hiding this comment

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

could you pls use the short version

SG_UNREF(results_one);
SG_UNREF(results_stop);
SG_UNREF(results);
SG_UNREF(results_complete);
Copy link
Member

Choose a reason for hiding this comment

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

no memory leaks/erriors in here?

Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there should not be since i only introduced bias change but i will check again

auto results_complete = svm_stop->apply_binary(test_features);

// compare model with completely trained reference
EXPECT_TRUE(results_complete->get_labels().equals(results->get_labels()));
Copy link
Member

Choose a reason for hiding this comment

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

you could actually call the clone method of SGObject here, then you do not need the get_labels() and can also use apply rather than apply_binary above

Copy link
Member

Choose a reason for hiding this comment

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

this is still true, also for the above label comparison

Copy link
Member

Choose a reason for hiding this comment

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

results_complete->equals(results)

Copy link
Member

Choose a reason for hiding this comment

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

(and then use apply instead of apply_binary


svm_stop->set_callback(nullptr);

// continue training till converged
Copy link
Member

Choose a reason for hiding this comment

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

until

EXPECT_TRUE(svm->train(features));
auto results = wrap(svm->apply(test_features));
auto acc = some<CAccuracyMeasure>();
EXPECT_EQ(acc->evaluate(results, test_labels), 1.0);
Copy link
Member

Choose a reason for hiding this comment

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

This is not the best unit test.
Better would be to compare the solver against liblinear or some reference implementation of the newton svm

auto perceptron_one = some<CPerceptron>();
perceptron_one->set_labels(labels);
perceptron_one->put<int32_t>("max_iterations", 1);
perceptron_one->set_compute_bias(false);
Copy link
Member

Choose a reason for hiding this comment

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

this can be removed

@shubham808 shubham808 force-pushed the feature/iterative_machine_tests branch from 7fa47df to 65a07f2 Compare August 5, 2018 13:28
@@ -122,13 +122,24 @@ TEST(Perceptron, continue_training_consistency)

ASSERT(!perceptron_stop->is_complete());

auto perceptron_one = some<CPerceptron>();
Copy link
Member

Choose a reason for hiding this comment

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

good!

#include <shogun/lib/Signal.h>

#include "environments/LinearTestEnvironment.h"
#include <gtest/gtest.h>
Copy link
Member

Choose a reason for hiding this comment

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

pls include gtest.h first, they use some magic macros that cause problems with some compilers otherwise

svm->set_labels(labels);
svm->train(features);

// reference for completly trained model
Copy link
Member

Choose a reason for hiding this comment

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

typo in comment

@shubham808 shubham808 force-pushed the feature/iterative_machine_tests branch from 65a07f2 to cf9737a Compare November 18, 2018 04:05
@shubham808
Copy link
Contributor Author

@karlnapf this solves the problem for now :) The CI seems to be acting weird but :/



/** Stores features to continue training */
CDotFeatures* m_continue_features;
Copy link
Member

Choose a reason for hiding this comment

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

I think this would need to be CFeatures*, or? Why would we restrict to CDotFeatures?
Sorry it has been a while, we might have discussed this before ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be CFeatures* but then we will need to check feature type in each iteration... for now we are only using IterativeMachine in LinearMachines which are DotFeatures.

Copy link
Member

Choose a reason for hiding this comment

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

I think we could get away with a static cast and rely on the fact that the algorithm knows the feature type is correct (inside iteration), since it was checked before the algorithm started (by your last piece of gsoc work)

@@ -32,7 +32,8 @@ namespace shogun
{
m_current_iteration = 0;
m_complete = false;

m_continue_features = NULL;
Copy link
Member

Choose a reason for hiding this comment

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

nullptr

@@ -57,7 +59,8 @@ namespace shogun
virtual bool continue_train()
{
this->reset_computation_variables();

Copy link
Member

Choose a reason for hiding this comment

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

let me understand this again. Is continue_train called from train? I.e. can we make sure the continure features are set on training start?

Copy link
Member

Choose a reason for hiding this comment

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

Or where exactly are those set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it is called from within train_machine

Copy link
Member

Choose a reason for hiding this comment

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

ok thanks!

Copy link
Member

@karlnapf karlnapf left a comment

Choose a reason for hiding this comment

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

What was the problem with this thing before?

Travis is currently down but will soon be up again, to be replaced by a microsoft service ... Stay tuned :)

@shubham808
Copy link
Contributor Author

The problem was we needed to make sure features do not change when we resume training. We have locally stored the features here that we will not be changed which solves our problem.

@karlnapf
Copy link
Member

karlnapf commented Dec 7, 2018

ok cool, so let's change the type to generic features and then merge this and move on ! :)

@shubham808 shubham808 force-pushed the feature/iterative_machine_tests branch from cf9737a to ed74e6d Compare December 9, 2018 09:48
@shubham808 shubham808 force-pushed the feature/iterative_machine_tests branch from ed74e6d to e174d4b Compare December 9, 2018 14:06
@karlnapf
Copy link
Member

Ok cool
PR CI builds are currently not active, so you will need to check all tests locally
If you give your ok we can merge (and then closely look at the CI and buildbot outputs)
Shall I? :)

@shubham808
Copy link
Contributor Author

alright ! lets merge it

@karlnapf karlnapf merged commit e18d0a5 into shogun-toolbox:develop Dec 11, 2018
@karlnapf
Copy link
Member

Ok pls have an eye on the buoldbot and the new azure builds

@karlnapf
Copy link
Member

@karlnapf
Copy link
Member

Cool!
Now what was next? :)

@shubham808
Copy link
Contributor Author

now lets look into the automated tests for this

@karlnapf
Copy link
Member

I have a local branch with an initial attempt on this, but not complete. Will share it

vigsterkr pushed a commit to vigsterkr/shogun that referenced this pull request Mar 9, 2019
* m_continue_features and iterative machine tests
ktiefe pushed a commit to ktiefe/shogun that referenced this pull request Jul 30, 2019
* m_continue_features and iterative machine tests
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.

None yet

2 participants