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

[ParametersObservers] Update CrossValidation with parameters observers. #3953

Merged
merged 9 commits into from Aug 16, 2017

Conversation

Projects
None yet
4 participants
@geektoni
Contributor

geektoni commented Jul 26, 2017

This patch relies on #3939, hence only the last three commits are relevant.

Changes:

  • Refactored CrossValidation to use the new obserevers, instead of the previous listeners;
  • Added CrossValidationStorage class which will store CrossValidation's information;
  • Added ParameterObserverCV to observer CrossValidation's parameters;
  • Added ParameterObserverCV tests;
  • Ported CrossValidationMKLStorage to ParameterObserverCVMKL;
  • Added a clear() method to ParameterObserverInterface to empty the observer from its observations
    (if implemented);
  • Added the new observers to SWIG;

Edit:

  • Removed CrossValidationPrintOutput and incorporate it into ParameterObserverCV;
  • Added ParameterObsverCVMulticlass;
  • Deleted CrossValidationOutput and CrossValidationMulticlassStorage;
  • Fixed some undocumented meta examples (libshogun and python);
@codecov

This comment has been minimized.

codecov bot commented Jul 26, 2017

Codecov Report

Merging #3953 into feature/parameter_observers_refactor will increase coverage by 0.07%.
The diff coverage is 79.06%.

Impacted file tree graph

@@                           Coverage Diff                           @@
##           feature/parameter_observers_refactor   #3953      +/-   ##
=======================================================================
+ Coverage                                 55.82%   55.9%   +0.07%     
=======================================================================
  Files                                      1356    1353       -3     
  Lines                                     94009   93997      -12     
=======================================================================
+ Hits                                      52484   52547      +63     
+ Misses                                    41525   41450      -75
Impacted Files Coverage Δ
src/shogun/lib/parameter_observers/ObservedValue.h 100% <ø> (+30.76%) ⬆️
src/shogun/classifier/mkl/MKLClassification.h 50% <ø> (ø) ⬆️
src/shogun/evaluation/CrossValidationStorage.h 0% <0%> (ø)
...b/parameter_observers/ParameterObserverInterface.h 0% <0%> (ø)
src/shogun/classifier/mkl/MKLClassification.cpp 67.85% <0%> (-27.15%) ⬇️
src/shogun/evaluation/CrossValidation.h 100% <100%> (ø) ⬆️
...parameter_observers/ParameterObserverInterface.cpp 53.12% <20%> (-29.23%) ⬇️
src/shogun/base/SGObject.cpp 71.25% <50%> (+2.8%) ⬆️
...un/lib/parameter_observers/ParameterObserverCV.cpp 74.64% <74.64%> (ø)
src/shogun/evaluation/CrossValidationStorage.cpp 83.33% <83.33%> (ø)
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34aaee5...1e4b35e. Read the comment docs.

@vigsterkr

This comment has been minimized.

Member

vigsterkr commented Aug 9, 2017

needs rebase :)

geektoni added some commits Jul 25, 2017

[ParametersObservers] Refactor CrossValidation to use parameter obser…
…vers.

Changes:
* Added two new classes CrossValidationStorage and CrossValidationFoldStorage,
  which will keep data when running CrossValidation;
* Added new parameter's observer for CrossValidation;
* Add new ObservedValue type;
* Add ParameterObserverCV tests;
[ParametersObservers] Replace old CrossValidationMKLStorage with a pa…
…rameter observer.

* Added a ParameterObserverCVMKL class which will replace the
  previous listener used.
* Correct an undocumented example to use this new object.
* Add a clear() method to ParameterObserverInterface which can be
  implemented to empty a parameter observer from its observations.
* Add observer to SWIG.
[ParametersObservers] Add Cross Validation Multiclass observer.
Other:
* Delete old CrossValidationMulticlassStorage code;
* Delete CrossValidationOutput class;
* Remove #pragma critial section from CrossValidation;
* Remove old files from SWIG and add the new parameter observer;
* Correct some python examples;
[ParametersObservers] Add reference counting to ParameterObserverInte…
…rface.

Update other observers and move ParameterObserverScalar unit test to
new directory.
[ParameterObservers] Delete specialized ParameterObserver classes (MK…
…L and Multiclass).

Since inside the observation we store, for each fold, the trained machine,
the user can just obtain the values from the saved machines.

Other:
* Refactor also the meta examples with these new features;
* Fix CrossValidation memory leak;
* Remove MKL and Multiclass classes from SWIG;
@geektoni

This comment has been minimized.

Contributor

geektoni commented Aug 14, 2017

@vigsterkr so this is almost complete. There are only two minor things which has to be addressed before merging it:

[ParameterObservers] Fix Python interface when dealing with CrossVali…
…dationStorage objects.

Other:
* Add obtain_from_generic method to CMKLClassification;
* Fix C++ meta example memory leak;
@@ -76,3 +79,6 @@
%include <shogun/evaluation/CrossValidationSplitting.h>
%include <shogun/evaluation/StructuredAccuracy.h>
%include <shogun/evaluation/DirectorContingencyTableEvaluation.h>

This comment has been minimized.

@vigsterkr

vigsterkr Aug 15, 2017

Member

namespace shogun ?

This comment has been minimized.

@geektoni

geektoni Aug 15, 2017

Contributor

It seems it is not needed to make things work.

@vigsterkr vigsterkr merged commit affb949 into shogun-toolbox:feature/parameter_observers_refactor Aug 16, 2017

4 checks passed

codecov/patch 79.06% of diff hit (target 55.82%)
Details
codecov/project 55.9% (+0.07%) compared to 34aaee5
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@karlnapf

I made some comments.
NICE work!

/* Copy the weights inside the matrix */
index_t run_shift =
fold->get_current_run_index() * w.vlen * o->get_num_folds();

This comment has been minimized.

@karlnapf

karlnapf Aug 16, 2017

Member

this method is a bit messy. I guess this can be done a bit cleaner?

weights = []
for o in mkl_storage.get_observations():
for fold in o.get_folds_results():
machine = MKLClassification.obtain_from_generic(fold.get_trained_machine())

This comment has been minimized.

@karlnapf

karlnapf Aug 16, 2017

Member

mmmh this cannot be translated into a meta example....
Maybe we can add an overly simplified one at least?
This one should go into the notebook on x-validation, we want to get rid of all language specific examples eventually

This comment has been minimized.

@geektoni

geektoni Aug 16, 2017

Contributor

I'm currently working on a simplified version (https://github.com/geektoni/shogun/blob/add_cross_validation_mkl_meta_example/examples/meta/src/evaluation/cross_validation_mkl_weight_storage.sg), which should show only the weights for one fold and one run (without cycling over all the observations).

# perform cross-validation
result=cross_validation.evaluate()
roc_0_0_0 = multiclass_storage.get_fold_ROC(0,0,0)
#roc_0_0_0 = multiclass_storage.get_fold_ROC(0,0,0)

This comment has been minimized.

@karlnapf
@@ -38,3 +38,16 @@ void CMKLClassification::init_training()
REQUIRE(m_labels->get_num_labels(), "Number of labels is zero.\n");
REQUIRE(m_labels->get_label_type() == LT_BINARY, "Labels must be binary.\n");
}
CMKLClassification* CMKLClassification::obtain_from_generic(CMachine* machine)

This comment has been minimized.

@karlnapf

karlnapf Aug 16, 2017

Member

@micmn wanna do some magic to create a static cast method for all Shogun classes?
Seems like you would know how to do that fast and then we could delete all the manual ones (and increase coverage) Thoughts?
@vigsterkr

This comment has been minimized.

@micmn

micmn Aug 18, 2017

Contributor

one issue I see is that the method declaration cannot be generated since it has to be in the header

using namespace shogun;
/**
* This test was inspired by the meta example

This comment has been minimized.

@karlnapf

karlnapf Aug 16, 2017

Member

should be inside one of the tests rather than in the outer file

This comment has been minimized.

@geektoni

geektoni Aug 16, 2017

Contributor

What do you mean here? :P Should I keep only the test and remove the meta example? Or it's better to port the meta example and keep also the test?

/* fill data matrix and labels */
SGMatrix<float64_t> train_dat(num_features, num_vectors);
SGVector<float64_t>::range_fill_vector(train_dat.matrix, num_vectors);
for (index_t i = 0; i < num_vectors; ++i)

This comment has been minimized.

@karlnapf

karlnapf Aug 16, 2017

Member

fixture class for test data pls, this is redundant code

return par;
}
TEST(ParameterObserverCV, get_result_locked)

This comment has been minimized.

@karlnapf

karlnapf Aug 16, 2017

Member

I guess "get_observations" would be more appropriate?

auto obs = par->get_observations();
for (int i = 0; i < 10; i++)
{
auto run = obs[i];

This comment has been minimized.

@karlnapf

karlnapf Aug 16, 2017

Member

ASSERT(run);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment