Removed some uses of m_model_selection_parameters [WIP] #4440

avramidis · 2018-12-17T09:14:02Z

This PR is part of the Parameter.cpp project.

In this PR we removed m_model_selection_parameters from SGObject. Also, in the future m_parameters and m_gradient_parameters will be removed. These store models' (hyper)parameters which are estimated during the training phase.

The aim is to use the AnyParameter properties to store the value and properties of these parameters. PRs #4412 and #4426 include code with the first steps towards this aim.

gf712

Are these all the occurrences of m_model_selection_parameters? What errors (if any) do you get when you compile this, and then run the tests?

gf712 · 2018-12-17T12:33:26Z

tests/unit/machine/gp/GaussianLikelihood_unittest.cc

@@ -268,8 +268,6 @@ TEST(GaussianLikelihood,get_first_derivative)
 	// Gaussian likelihood with sigma = 0.13
 	CGaussianLikelihood* likelihood=new CGaussianLikelihood(0.13);

-	TParameter* param=likelihood->m_model_selection_parameters->get_parameter("log_sigma");
-
 	SGVector<float64_t> lp_dhyp=likelihood->get_first_derivative(labels, func,
 			param);



I am assuming the compiler complains that param isn't declared here right?

gf712 · 2018-12-17T12:34:44Z

src/shogun/base/SGObject.h

@@ -92,8 +92,6 @@ template <class T> class SGStringList;
 		    AnyParameterProperties(description, param_properties);             \
 		this->m_parameters->add(param, name, description);                     \
 		this->watch_param(name, param, pprop);                                 \
-		if (pprop.get_model_selection())                                       \
-			this->m_model_selection_parameters->add(param, name, description); \
 		if (pprop.get_gradient())                                              \
 			this->m_gradient_parameters->add(param, name, description);        \
 	}


Did you try to remove m_gradient_parameters yet? Just curious as to how many things break..

No, I didn't remove m_gradient_parameters. Mostly the model selection functionality breaks.

avramidis · 2018-12-17T14:42:29Z

The errors I get is mostly complains from the compiler that the m_model_selection_parameters cannot be found.

gf712 · 2018-12-17T15:31:24Z

OK! I think the idea is to now use CSGObject::get_parameter instead, and from there you can extract (maybe cast?) the value of the parameter. But I guess we'll discuss that tomorrow!

karlnapf · 2018-12-18T11:15:21Z

examples/undocumented/libshogun/parameter_modsel_parameters.cpp

@@ -23,8 +23,6 @@ void print_message(FILE* target, const char* str)

 void print_modsel_parameters(CSGObject* object)


we can just drop this function

actually, we can just drop the whole cpp example ... to be replaced with a meta example

Actually
@gf712 could you try to write a meta example for base_api where you show how to extract model selection parameters at some point? mayber after the monkey business is done

Yes, I can add that to #4441

karlnapf · 2018-12-18T11:18:25Z

src/shogun/base/SGObject.cpp

@@ -657,27 +573,6 @@ void CSGObject::get_parameter_incremental_hash(uint32_t& hash, uint32_t& carry,
 	}
 }

-void CSGObject::build_gradient_parameter_dictionary(CMap<TParameter*, CSGObject*>* dict)


this we need to keep working somehow as it is used for the GP framework ... so a replacement needs to be written
The signature will probably change as TParameter will be removed and we will use the dispatching code that @gf712 wrote

Yup, should be pretty straightforward to replace this, might be worth waiting for #4432 to be done

karlnapf · 2018-12-18T11:19:12Z

src/shogun/modelselection/GridSearchModelSelection.cpp

@@ -68,9 +68,6 @@ CParameterCombination* CGridSearchModelSelection::select_model(bool print_state)
 			current_combination->print_tree();
 		}

-		current_combination->apply_to_modsel_parameter(


this is the big fish to replace

Could you, instead of removing the method call, just keep it and place a runtime error (SG_NOTIMPLEMENTED) inside the method?

karlnapf · 2018-12-18T11:20:45Z

src/shogun/modelselection/ParameterCombination.cpp

@@ -700,7 +698,7 @@ CParameterCombination* CParameterCombination::copy_tree() const

 void CParameterCombination::apply_to_machine(CMachine* machine) const
 {
-	apply_to_modsel_parameter(machine->m_model_selection_parameters);


same here. The danger with just removing this line is that we forget to re-add it later when we have a replacement. So rather than changing behaviour of code, let's just put a runtime error

karlnapf · 2018-12-18T11:20:49Z

src/shogun/modelselection/ParameterCombination.cpp

@@ -749,8 +747,6 @@ void CParameterCombination::apply_to_modsel_parameter(
 			{
 				CParameterCombination* child=(CParameterCombination*)
 						m_child_nodes->get_element(i);
-				child->apply_to_modsel_parameter(


karlnapf · 2018-12-18T11:21:26Z

tests/unit/machine/gp/GaussianLikelihood_unittest.cc

@@ -268,8 +268,6 @@ TEST(GaussianLikelihood,get_first_derivative)
 	// Gaussian likelihood with sigma = 0.13
 	CGaussianLikelihood* likelihood=new CGaussianLikelihood(0.13);

-	TParameter* param=likelihood->m_model_selection_parameters->get_parameter("log_sigma");


Looking at the test, this can probably already be replaced with get

karlnapf · 2018-12-18T11:21:42Z

tests/unit/machine/gp/StudentsTLikelihood_unittest.cc

@@ -270,9 +270,6 @@ TEST(StudentsTLikelihood,get_first_derivative)
 	// Stundent's-t likelihood with sigma = 0.17, df = 3
 	CStudentsTLikelihood* likelihood=new CStudentsTLikelihood(0.17, 3);

-	TParameter* param1=likelihood->m_model_selection_parameters->get_parameter("log_sigma");


in all of those actually, get should do it

avramidis · 2019-01-10T09:43:01Z

A very dirty update with mistakes. The next step would be to remove Parameter, ParameterCombination ,however, many stuff will break. I will have to change (& simplify) the hyper-parameter optimisation which I prefer to do.

karlnapf · 2019-01-13T09:49:43Z

src/shogun/modelselection/ModelSelectionParameter.h

+
+namespace shogun
+{
+


I am not sure I agree with this idea of making a class for modelselection parameters....we will have to discuss a bit here.
But actually, as discussed a few day back in person, let's maybe start with the gradient parameter API refactoring

stale · 2020-02-26T15:52:27Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

gf712 · 2020-02-26T16:37:34Z

this has already been done

gf712 reviewed Dec 17, 2018

View reviewed changes

karlnapf reviewed Dec 18, 2018

View reviewed changes

Removed some uses of m_model_selection_parameters

7d50a9b

avramidis force-pushed the feature/remove-m_model_selection_parameters branch from b51e2b1 to 7d50a9b Compare January 10, 2019 09:38

karlnapf reviewed Jan 13, 2019

View reviewed changes

stale bot added the stale label Feb 26, 2020

gf712 closed this Feb 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed some uses of m_model_selection_parameters [WIP] #4440

Removed some uses of m_model_selection_parameters [WIP] #4440

avramidis commented Dec 17, 2018

gf712 left a comment

gf712 Dec 17, 2018

gf712 Dec 17, 2018

avramidis Dec 17, 2018

avramidis commented Dec 17, 2018

gf712 commented Dec 17, 2018

karlnapf Dec 18, 2018

karlnapf Dec 18, 2018

gf712 Dec 18, 2018

karlnapf Dec 18, 2018

gf712 Dec 18, 2018

karlnapf Dec 18, 2018

karlnapf Dec 18, 2018

karlnapf Dec 18, 2018

karlnapf Dec 18, 2018

karlnapf Dec 18, 2018

karlnapf Dec 18, 2018

avramidis commented Jan 10, 2019

karlnapf Jan 13, 2019

stale bot commented Feb 26, 2020

gf712 commented Feb 26, 2020

		@@ -23,8 +23,6 @@ void print_message(FILE* target, const char* str)

		void print_modsel_parameters(CSGObject* object)


		namespace shogun
		{

Removed some uses of m_model_selection_parameters [WIP] #4440

Removed some uses of m_model_selection_parameters [WIP] #4440

Conversation

avramidis commented Dec 17, 2018

gf712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avramidis commented Dec 17, 2018

gf712 commented Dec 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avramidis commented Jan 10, 2019

Choose a reason for hiding this comment

stale bot commented Feb 26, 2020

gf712 commented Feb 26, 2020