SG_ADD refactor #4417

gf712 · 2018-11-16T15:09:18Z

Now that we can register parameter properties with a single enum we don't need to have several SG_ADD macros with variable number of parameters.
I replaced with a regular expression the following expressions in the whole project:

MS_NOT_AVAILABLE()); -> ParameterProperties());
MS_AVAILABLE()); -> ParameterProperties::HYPER);
MS_AVAILABLE, GRADIENT_AVAILABLE); -> ParameterProperties::HYPER | ParameterProperties::GRADIENT);

We would have to do the same with the code in the GPL submodule.
Sorry about the large commit, it just seemed like replacing everything with a regex was the simplest!
I also tested the code locally to make sure it works fine, and passed the tests.

* with the new AnyParameterProperties we can clean up the macros a bit and have a single macro with four parameters * inside the macro we still have exactly the same process

karlnapf

A few initial comments. Main change is I think we should make default properties less verbose

karlnapf · 2018-11-16T15:29:57Z

src/shogun/base/SGObject.h

@@ -4,7 +4,8 @@
 * Authors: Heiko Strathmann, Soeren Sonnenburg, Sergey Lisitsyn,
 *          Giovanni De Toni, Jacob Walker, Thoralf Klein, Chiyuan Zhang,
 *          Fernando Iglesias, Sanuj Sharma, Roman Votyakov, Yuyu Zhang,
- *          Viktor Gal, Bjoern Esser, Evangelos Anagnostopoulos, Pan Deng
+ *          Viktor Gal, Bjoern Esser, Evangelos Anagnostopoulos, Pan Deng,
+ *          Gil Hoben


Weclome to SGObject ;)

Haha yes, I thought I would discretely add myself to the list :D

karlnapf · 2018-11-16T15:33:45Z

src/shogun/classifier/LDA.cpp

-	SG_ADD(&m_bdc_svd, "m_bdc_svd", "Use BDC-SVD algorithm", MS_NOT_AVAILABLE);
+	    "Method used for LDA calculation", ParameterProperties());
+	SG_ADD(&m_gamma, "m_gamma", "Regularization parameter", ParameterProperties::HYPER);
+	SG_ADD(&m_bdc_svd, "m_bdc_svd", "Use BDC-SVD algorithm", ParameterProperties());


Mmmh. This is not nice, it would be better if this default was done automatically.
I didn't realise this before, but I think it means we have to have SG_ADD3 (default properties) and SG_ADD4(user specified properties) as macros cannot do default parameters.

karlnapf · 2018-11-16T15:34:13Z

src/shogun/base/SGObject.h

@@ -59,50 +60,18 @@ template <class T> class SGStringList;
 * Macros for registering parameters/model selection parameters
 ******************************************************************************/

-#ifdef _MSC_VER
-
-#define VA_NARGS(...)  INTERNAL_EXPAND_ARGS_PRIVATE(INTERNAL_ARGS_AUGMENTER(__VA_ARGS__))


While it would be good to get rid of this thing, I think we still need to keep it (see below on default properties)

OK, I will bring this back and add a SG_ADD3!

karlnapf · 2018-11-16T15:34:37Z

src/shogun/classifier/LDA.cpp

-	SG_ADD(&m_gamma, "m_gamma", "Regularization parameter", MS_AVAILABLE);
-	SG_ADD(&m_bdc_svd, "m_bdc_svd", "Use BDC-SVD algorithm", MS_NOT_AVAILABLE);
+	    "Method used for LDA calculation", ParameterProperties());
+	SG_ADD(&m_gamma, "m_gamma", "Regularization parameter", ParameterProperties::HYPER);


this is quite nice now

karlnapf · 2018-11-16T15:35:10Z

src/shogun/classifier/PluginEstimate.cpp

@@ -22,17 +22,17 @@ CPluginEstimate::CPluginEstimate(float64_t pos_pseudo, float64_t neg_pseudo)
 {
 	SG_ADD(
 	    &m_pos_pseudo, "pos_pseudo", "pseudo count for positive class",
-	    MS_NOT_AVAILABLE);
+	    ParameterProperties());


again, would be good if default was taken if just nothing was provided

karlnapf · 2018-11-16T15:35:48Z

src/shogun/classifier/mkl/MKL.cpp

+	SG_ADD(&C_mkl, "C_mkl", "C mkl", ParameterProperties());
+	SG_ADD(&mkl_norm, "mkl_norm", "norm used in mkl", ParameterProperties());
+	SG_ADD(&ent_lambda, "ent_lambda", "elastic net sparsity trade-off parameter", ParameterProperties());
+	SG_ADD(&mkl_block_norm, "mkl_block_norm", "mkl sparse trade-off parameter", ParameterProperties());


karlnapf · 2018-11-16T15:36:49Z

src/shogun/machine/LinearMachine.cpp

@@ -35,11 +35,11 @@ void CLinearMachine::init()
 	bias = 0;
 	features = NULL;

-	SG_ADD(&m_w, "w", "Parameter vector w.", MS_NOT_AVAILABLE);
-	SG_ADD(&bias, "bias", "Bias b.", MS_NOT_AVAILABLE);
+	SG_ADD(&m_w, "w", "Parameter vector w.", ParameterProperties());


things like this would be MODEL

Both the weight and bias right?

yes. We dont need to fix all the wrongly added parameter in this PR though, just put a few examples in

karlnapf · 2018-11-16T15:36:56Z

src/shogun/machine/LinearStructuredOutputMachine.cpp

@@ -68,7 +68,7 @@ CStructuredLabels* CLinearStructuredOutputMachine::apply_structured(CFeatures* d

 void CLinearStructuredOutputMachine::register_parameters()
 {
-	SG_ADD(&m_w, "m_w", "Weight vector", MS_NOT_AVAILABLE);
+	SG_ADD(&m_w, "m_w", "Weight vector", ParameterProperties());


karlnapf · 2018-11-16T15:37:12Z

src/shogun/machine/OnlineLinearMachine.cpp

@@ -19,10 +19,10 @@ using namespace shogun;
 COnlineLinearMachine::COnlineLinearMachine()
 : CMachine(), bias(0), features(NULL)
 {
-	SG_ADD(&m_w, "m_w", "Parameter vector w.", MS_NOT_AVAILABLE);
-	SG_ADD(&bias, "bias", "Bias b.", MS_NOT_AVAILABLE);
+	SG_ADD(&m_w, "m_w", "Parameter vector w.", ParameterProperties());


model, you get the hang :)

karlnapf · 2018-11-16T15:37:33Z

src/shogun/machine/gp/GaussianLikelihood.cpp

@@ -52,7 +52,7 @@ CGaussianLikelihood::CGaussianLikelihood(float64_t sigma) : CLikelihoodModel()
 void CGaussianLikelihood::init()
 {
 	m_log_sigma=0.0;
-	SG_ADD(&m_log_sigma, "log_sigma", "Observation noise in log domain", MS_AVAILABLE, GRADIENT_AVAILABLE);
+	SG_ADD(&m_log_sigma, "log_sigma", "Observation noise in log domain", ParameterProperties::HYPER | ParameterProperties::GRADIENT);


also refactored the SG_ADD calls respectively

karlnapf · 2018-11-16T19:50:42Z

src/shogun/base/SGObject.h

+	{                                                                          \
+		this->m_parameters->add(param, name, description);                     \
+		this->watch_param(                                                     \
+		    name, param, AnyParameterProperties()); 						   \


yes this is good

karlnapf · 2018-11-16T19:50:56Z

src/shogun/classifier/LDA.cpp

@@ -49,9 +49,9 @@ void CLDA::init()

 	SG_ADD(
 	    (machine_int_t*)&m_method, "m_method",
-	    "Method used for LDA calculation", ParameterProperties());
+	    "Method used for LDA calculation");


yep, much cleaner

karlnapf · 2018-11-16T19:53:16Z

src/shogun/converter/LocallyLinearEmbedding.cpp

 	SG_ADD(&m_nullspace_shift, "nullspace_shift",
-      "nullspace finding regularization shift",MS_NOT_AVAILABLE);
+      "nullspace finding regularization shift",ParameterProperties());


missed one here :)

karlnapf · 2018-11-16T19:53:26Z

src/shogun/converter/ManifoldSculpting.cpp

 	SG_ADD(&m_squishing_rate, "quishing_rate",
-      "squishing rate",MS_NOT_AVAILABLE);
+      "squishing rate",ParameterProperties());


…ctor

karlnapf · 2018-11-17T16:27:08Z

src/shogun/base/SGObject.h

-		                     description, ms_available, gradient_available));  \
-		if (ms_available)                                                      \
+		this->watch_param(name, param, pprop);                                 \
+		if (pprop.get_model_selection())                                       \


Note for later: we can already start to get rid of the model_selection_parameters field and instead make the code work with any. Definitely a sub-task in all this refactoring ... but more on it later

karlnapf · 2018-11-17T16:27:20Z

src/shogun/base/SGObject.h

 			this->m_model_selection_parameters->add(param, name, description); \
-		if (gradient_available)                                                \
+		if (pprop.get_gradient())                                              \


same for those gradient parameters

karlnapf · 2018-11-29T20:41:40Z

BTW this passed all of our CI
https://dev.azure.com/shogunml/shogun/_build/results?buildId=91

Try to keep an eye on the output of this after any patch is merged (until we have the pre-merge builds back)

gf712 · 2018-11-30T10:39:13Z

Yes, I saw that. I think it's because of the multiple constructors, which also have defaults, led to something we didn't expect. So might be worth removing the constructors which accept the old enums since they shouldn't be required anymore?

karlnapf · 2018-11-30T11:08:05Z

Sorry what do you mean? :)

gf712 · 2018-11-30T11:14:03Z

We still have these in the code:

/** model selection availability */
enum EModelSelectionAvailability
{
	MS_NOT_AVAILABLE = 0,
	MS_AVAILABLE = 1,
};

/** gradient availability */
enum EGradientAvailability
{
	GRADIENT_NOT_AVAILABLE = 0,
	GRADIENT_AVAILABLE = 1
};

Might be worth removing this with an update of the hash for the latest commit on the GPL repo?

I am not sure we are talking about the same thing though. I thought you meant you expected some tests to have failed because the GPL repo hasn't been updated yet on the shogun repo? Which they should have, because GPL still uses the old SG_ADD, right?

karlnapf · 2018-11-30T11:47:00Z

ah I see now.

GPL should compile, could you check/make sure?

removing these might cause a lot of compile errors, we should eventually do that (as I said, the modelselection parameters should be removed as well), but maybe for now let's focus on one thing?

gf712 · 2018-11-30T14:12:04Z

GPL compiles fine! And the tests pass.

* Replaced the SG_ADD macro calls with the new ParameterProperties enum class * this was done in the same way as in PR shogun-toolbox/shogun#4417

karlnapf · 2018-11-30T15:26:30Z

cool thanks!

* Replaced the SG_ADD macro calls with the new ParameterProperties enum class * this was done in the same way as in PR shogun-toolbox/shogun#4417

* replaced variable macro with a single macro [ci skip] * with the new AnyParameterProperties we can clean up the macros a bit and have a single macro with four parameters * inside the macro we still have exactly the same process * refactored all SG_ADD macros to use ParameterProperties [ci skip] * added SG_ADD3 for default parameters [ci skip] also refactored the SG_ADD calls respectively

Gil added 2 commits November 16, 2018 15:01

replaced variable macro with a single macro [ci skip]

6de1d81

* with the new AnyParameterProperties we can clean up the macros a bit and have a single macro with four parameters * inside the macro we still have exactly the same process

refactored all SG_ADD macros to use ParameterProperties [ci skip]

c610f3c

karlnapf requested changes Nov 16, 2018

View reviewed changes

Gil added 2 commits November 16, 2018 16:33

added SG_ADD3 for default parameters [ci skip]

d304e15

also refactored the SG_ADD calls respectively

started adding param properties [ci skip]

a601c0a

karlnapf reviewed Nov 16, 2018

View reviewed changes

Gil added 3 commits November 16, 2018 22:18

refactored SG_ADD3 calls

bd9e09e

refactored SG_ADD3 calls [ci skip]

2458eee

Merge remote-tracking branch 'gf712/sg_add_refactor' into sg_add_refa…

9d6e506

…ctor

karlnapf reviewed Nov 17, 2018

View reviewed changes

karlnapf approved these changes Nov 21, 2018

View reviewed changes

karlnapf merged commit b5345f5 into shogun-toolbox:develop Nov 21, 2018

gf712 deleted the sg_add_refactor branch November 21, 2018 17:22

gf712 mentioned this pull request Nov 22, 2018

SG_ADD refactor shogun-toolbox/shogun-gpl#8

Merged

gf712 pushed a commit to gf712/shogun-gpl that referenced this pull request Nov 30, 2018

sg_add_refctor:

d73042f

* Replaced the SG_ADD macro calls with the new ParameterProperties enum class * this was done in the same way as in PR shogun-toolbox/shogun#4417

gf712 pushed a commit to gf712/shogun-gpl that referenced this pull request Nov 30, 2018

sg_add_refactor:

4a16d71

* Replaced the SG_ADD macro calls with the new ParameterProperties enum class * this was done in the same way as in PR shogun-toolbox/shogun#4417

gf712 pushed a commit to gf712/shogun-gpl that referenced this pull request Dec 3, 2018

sg_add refactor on GPL branch

5567f40

* Replaced the SG_ADD macro calls with the new ParameterProperties enum class * this was done in the same way as in PR shogun-toolbox/shogun#4417

gf712 pushed a commit to gf712/shogun-gpl that referenced this pull request Dec 3, 2018

sg_add refactor on GPL branch

aeb868e

* Replaced the SG_ADD macro calls with the new ParameterProperties enum class * this was done in the same way as in PR shogun-toolbox/shogun#4417

gf712 pushed a commit to gf712/shogun-gpl that referenced this pull request Dec 3, 2018

sg_add refactor on GPL branch

ae5a431

* Replaced the SG_ADD macro calls with the new ParameterProperties enum class * this was done in the same way as in PR shogun-toolbox/shogun#4417

gf712 pushed a commit to gf712/shogun-gpl that referenced this pull request Dec 3, 2018

sg_add refactor on GPL branch

35a84e9

* Replaced the SG_ADD macro calls with the new ParameterProperties enum class * this was done in the same way as in PR shogun-toolbox/shogun#4417

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SG_ADD refactor #4417

SG_ADD refactor #4417

gf712 commented Nov 16, 2018

karlnapf left a comment

karlnapf Nov 16, 2018

gf712 Nov 16, 2018

karlnapf Nov 16, 2018

karlnapf Nov 16, 2018

gf712 Nov 16, 2018

karlnapf Nov 16, 2018

karlnapf Nov 16, 2018

karlnapf Nov 16, 2018

karlnapf Nov 16, 2018

gf712 Nov 16, 2018

karlnapf Nov 16, 2018

karlnapf Nov 16, 2018

karlnapf Nov 16, 2018

karlnapf Nov 16, 2018

karlnapf Nov 16, 2018

karlnapf Nov 16, 2018

karlnapf Nov 16, 2018

karlnapf Nov 16, 2018

karlnapf Nov 17, 2018

karlnapf Nov 17, 2018

karlnapf commented Nov 29, 2018

gf712 commented Nov 30, 2018

karlnapf commented Nov 30, 2018

gf712 commented Nov 30, 2018

karlnapf commented Nov 30, 2018

gf712 commented Nov 30, 2018

karlnapf commented Nov 30, 2018

SG_ADD refactor #4417

SG_ADD refactor #4417

Conversation

gf712 commented Nov 16, 2018

karlnapf 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

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

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

karlnapf commented Nov 29, 2018

gf712 commented Nov 30, 2018

karlnapf commented Nov 30, 2018

gf712 commented Nov 30, 2018

karlnapf commented Nov 30, 2018

gf712 commented Nov 30, 2018

karlnapf commented Nov 30, 2018