AnyParameterProperties refactor #4412

gf712 · 2018-11-07T15:10:00Z

This PR refactors the AnyParameterProperties class to use a mask (m_mask_attribute) to store parameter options:

HYPERPARAMETER: the parameter determines how the training is performed, e.g. regularisation
GRADIENT_PARAM: the parameter is used in gradient updates
MODEL_PARAM: model parameter that is trained, e.g. weight and bias
This makes the use of Enums in AnyParameter.h not necessary anymore, and the corresponding getter methods can be replaced with bool return type.
The mask uses bit shift operations to keep information in a hexadecimal representation.

* added mask_attribute to keep track of parameter availabilities * updated constructors and getters * added some documentation

`AnyParameterProperties::HYPERPARAMETER | AnyParameterProperties::GRADIENT_PARAM | AnyParameterProperties::MODELSELECTION_PARAM` is equivalent to all parameter property flags being false (`0x00000000`)

modelselection_param is actually hyperparameter now and model_param is a seperate definition, e.g. bias and weights

lisitsyn · 2018-11-07T15:38:17Z

src/shogun/base/AnyParameter.h

-		AnyParameterProperties()
-		    : m_description(), m_model_selection(MS_NOT_AVAILABLE),
-		      m_gradient(GRADIENT_NOT_AVAILABLE)
+		static const int32_t HYPERPARAMETER = 0x00000001;


What about making it an int enum?

Check this
https://stackoverflow.com/questions/4920783/combining-enum-value-using-bitmask

Ah that's neat! I'll implement that.

lisitsyn · 2018-11-07T15:39:18Z

src/shogun/base/AnyParameter.h

-			return m_gradient;
+			EGradientAvailability return_val;
+			if (m_mask_attribute & GRADIENT_PARAM)
+				return_val = EGradientAvailability::GRADIENT_AVAILABLE;


it makes sense to return immediately without return_val

Do you mean with a one liner like this?

return static_cast<EGradientAvailability>((m_mask_attribute & GRADIENT_PARAM) > 0);

Yeah maybe with some helper function so it looks shorter? There is some repeated code in these flags.

karlnapf · 2018-11-07T20:55:51Z

src/shogun/base/AnyParameter.h

+	/** @brief Class AnyParameterProperties keeps track of parameter properties.
+	 *
+	 * Currently there are three types of parameters:
+	 * -# HYPERPARAMETER: the parameter determines how the training is


Pls don’t put such things into the docs, they outdated at light speed.
Better describe that there are attributes, maybe an example, but not losing them all

karlnapf · 2018-11-07T20:56:39Z

src/shogun/base/AnyParameter.h

-		    : m_description(), m_model_selection(MS_NOT_AVAILABLE),
-		      m_gradient(GRADIENT_NOT_AVAILABLE)
+		static const int32_t HYPERPARAMETER = 0x00000001;
+		static const int32_t GRADIENT_PARAM = 0x00000010;


You can also use the << operator to shift the 1 a desired number of digits
See the stack overflow post

I had a look at the post, and the Enum with bit operations is pretty cool. I just thought that the hexadecimal representation is more readable than doing the bit shifts, no?

at the end of the day, it is style. You can either define the mask as you did with hexadecimal as you did there. Or, instead, you can generate those masks on the fly using a shift operator, and then the HYPERPARAMETER constant is the number of bits that you shift, i.e.

my_mask && (1 << HYPERPARAMETER) && (1 << GRADIENT_AVAILABLE) &&

The latter has the advantage that somebody reading the code never is exposed to hexadecimal numbers, nor powers or two (which can be confusing). On the other hand, masking operations (as what I wrote above) becomes a bit more notation heavy. Idk what is best to be honest. I think the shifting is more standard and therefore I think we should follow that.

Ah one thing I think is good about the shifting approach is that you can change the size of the mask easily. You are using a 32bit int above, but you only write down the last 8 bits, which is weird. Imagine you now changed the mask to 64 bit. Then the hexadecimals would be quite long, whereas the shifts wouldnt be.

@lisitsyn @vigsterkr ?

OK! I think you're right, I have changed the class members to use bit shifts, it is a bit cleaner.

karlnapf · 2018-11-08T11:04:48Z

src/shogun/base/AnyParameter.h

+		static const int32_t GRADIENT_PARAM = 0x00000010;
+		static const int32_t MODEL_PARAM = 0x00000100;
+
+		/** default constructor where all parameter types are switched to false


Minor: sentences start with capital letters

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

src/shogun/base/AnyParameter.h

 		{
 		}
+		/** legacy constructor */


I would remove that. Until we haven't removed it, it is not legacy.

karlnapf · 2018-11-08T11:06:20Z

src/shogun/base/AnyParameter.h

 		}

 	private:
 		std::string m_description;
 		EModelSelectionAvailability m_model_selection;
 		EGradientAvailability m_gradient;
+		/** mask to store all param flags*/
+		int32_t m_mask_attribute;


what about calling it "attribute_mask" ? :)

Yup, I just noticed that I switched the order...

karlnapf · 2018-11-08T11:06:57Z

src/shogun/base/AnyParameter.h

+			if (hyperparameter)
+				m_mask_attribute |= HYPERPARAMETER;
+		}
+		/** mask constructor */


if you add doxygen docs, pls document all parameters
minor: capital M

karlnapf · 2018-11-08T11:08:19Z

src/shogun/base/AnyParameter.h

 		    : m_description(description), m_model_selection(model_selection),
 		      m_gradient(gradient)
 		{
+			m_mask_attribute = 0x00000000;
+			if (model_selection)


wouldnt this work?

m_attribute_mask = (MODEL_PARAM & model_selection) | (GRADIENT_PARAM & gradient) | (HYPERPARAMETER & hyperparameter)

or in the new style

m_attribute_mask = ((1<< MODEL_PARAM) & model_selection) | ((1<< GRADIENT_PARAM) & gradient) | ((1<<HYPERPARAMETER) & hyperparameter)

True, that would be more concise. Is it worth writing a static member that is the default, i.e. 0?

If the default is 0, then we don't need that.
I would be explicit about a non-zero default, and add a private init method that sets it and that is called by all ctors.

karlnapf · 2018-11-08T11:09:59Z

src/shogun/base/AnyParameter.h

-		    : m_description(), m_model_selection(MS_NOT_AVAILABLE),
-		      m_gradient(GRADIENT_NOT_AVAILABLE)
+		static const int32_t HYPERPARAMETER = 0x00000001;
+		static const int32_t GRADIENT_PARAM = 0x00000010;


I would call this GRADIENT_AVAILABLE

karlnapf · 2018-11-08T11:11:56Z

src/shogun/base/AnyParameter.h

-		AnyParameterProperties()
-		    : m_description(), m_model_selection(MS_NOT_AVAILABLE),
-		      m_gradient(GRADIENT_NOT_AVAILABLE)
+		static const int32_t HYPERPARAMETER = 0x00000001;


can we keep the naming self-consistent
HYPER_PARAM
or
HYPER_PARAMETER
or name the other one
MODELPARAMETER
or rather
MODEL_PARAMETER

Actually, in favour of short and concise names, why dont we use
HYPER
MODEL
GRADIENT
?
And then the constant itself can have a doxygen string to say what that means?

OK! But hyperparameter is a term that could be misinterpreted if it written as HYPER, no?

I think it is pretty clear since the class is ending with "Parameter", so everything in there is one ....

The name MODEL causes conflicts if we use an enum, so will have to use something more specific, or just have these values as static class members of AnyParameterProperties.

yes, please don't define them globally, but as a static member

would it make sense to use an enum within the class scope?

fine as well

karlnapf · 2018-11-08T11:12:29Z

src/shogun/base/AnyParameter.h

 		{
 		}
-
+		/** description getter */


superflous doc, as the method name explains it, i would just remove it

karlnapf · 2018-11-08T11:13:02Z

src/shogun/base/AnyParameter.h

 		{
 		}
-
+		/** description getter */
 		std::string get_description() const


minor what about returning a const string reference

Yes, I think we could do that. It seems to be pretty much standard now to return const references for std::string. It can sometimes lead to odd behaviours, but I don't think we need to worry about those..
http://thesyntacticsugar.blogspot.com/2011/09/evil-side-of-returning-member-as-const.html

yes, but the first rule of the C++ committee is:
"The C++ language does not prevent people from shooting themselves in their feet" ;)

karlnapf · 2018-11-08T11:13:15Z

src/shogun/base/AnyParameter.h

 		std::string get_description() const
 		{
 			return m_description;
 		}
-
+		/** model selection flag getter */


same here, would remove the comment

karlnapf · 2018-11-08T11:14:33Z

src/shogun/base/SGObject.h

@@ -676,7 +676,10 @@ class CSGObject
 	void watch_param(
 		const std::string& name, T* value,
 		AnyParameterProperties properties = AnyParameterProperties(
-		    "Unknown parameter", MS_NOT_AVAILABLE, GRADIENT_NOT_AVAILABLE))
+			"Unknown parameter",
+			AnyParameterProperties::HYPERPARAMETER |


instead of doing this here, I think it might be better to offer a constructor that does not take any properties, and then set the default values inside there. We need to discuss clever default values as well btw

karlnapf · 2018-11-08T11:15:51Z

src/shogun/base/SGObject.h

@@ -715,7 +721,10 @@ class CSGObject
 	void watch_param(
 		const std::string& name, T** value, S* rows, S* cols,
 		AnyParameterProperties properties = AnyParameterProperties(
-		    "Unknown parameter", MS_NOT_AVAILABLE, GRADIENT_NOT_AVAILABLE))
+			"Unknown parameter",


Same with the description, I would offer a ctor that does not ask for one, and then set it to the empty string or "No description given" in the ctor, but not here in SGObject.h

karlnapf

Great first start. We need a few changes and clean-ups, but this is essentially doing what we want already.
Maybe we can have a test that checks the bitmasking for one of the parameters ... just to be on the safe side ... :)

* also shorted class member names * changed default constructor to give default description

karlnapf · 2018-11-09T12:34:52Z

src/shogun/base/AnyParameter.h

-	 * -# GRADIENT_PARAM: the parameter is used in gradient updates
-	 * -# MODEL_PARAM: model parameter that is trained, e.g. weight and bias
-	 * vectors and bias
+	 * The parameter properties can be either true or false.


like!
Maybe also mention descriptions in the first sentence " ... of parameter meta information, such as properties and descriptions"

karlnapf · 2018-11-09T12:37:09Z

src/shogun/base/AnyParameter.h

-		static const int32_t MODEL_PARAM = 0x00000100;
+		static const int32_t HYPER = 1u << 0;
+		static const int32_t GRADIENT = 1u << 1;
+		static const int32_t MODEL = 1u << 2;


ah wait. the thing I had in mind was

static const int HYPER = 1;

and then

mask = (1 << HYPER) | (1 << MODEL);

This is now difference, as you are effectively storing masks themselves in these const members, not the number of bit shifts. Or am I wrong? Let me stare a bit more to understand

Do you mean:

static const int HYPER = 0; static const int GRADIENT = 1; static const int MODEL = 2;

yes, but start at 1

OK, if we have:

static const int HYPER = 1; static const int GRADIENT = 2; static const int MODEL = 3; bool model_selection = true; bool gradient = true; int mask = ((1 << HYPER) & model_selection) | ((1 << GRADIENT) & gradient);

Then the above mask will always be false/0 no?

Do you mean more like this:

int mask = (model_selection << HYPER) | (gradient << GRADIENT)

which is the equivalent of: shift bit at position X (1,2 or 3 in this case) if the function parameter is true.

Ah now I remember why I did it this way.. Basically if you want to provide an interface to create your own mask like this

AnyParameterProperties("My description", AnyParameterProperties::MODEL | AnyParameterProperties::GRADIENT)

you need to have the values of the static member with the shift. Unless I am missing something?

Not sure what you mean, but what I have seen (and been doing) would result in

AnyParameterProperties("My description", (1 << AnyParameterProperties::MODEL) | (1 << AnyParameterProperties::GRADIENT))

It is not as neat, you are totally right!. BUT it can be hidden quite easily either.
I mean, it is your code, Ill leave it to you. Not sure whether it is possible to say what is better now, we will have to commit to something and then see if it works :) So go ahead with the above solution and then we see where we get

OK! Btw why does it need to start at 1, instead of 0?

ah, no that was nonsense (just needed to make sure that you never get an all zero mask)

BTW I also realised why the micro-controller people store the number of bits rather than the full masks: it uses less memory (which is not really relevant here, but still good to think about it for a moment).

byte HYPER = 2 ... byte BLA = 255

so with an 8bit integer, you can do 255 attributes if you generate the masks on the fly a la (1 << HYPER). If you wanted to store the mask for each attribute instead (i.e. HYPER = 0x00000010 (each mask is 8bit), you would have to store 255 * 8bit, which is quite a difference , especially if you have only 4k memory as on an AVR ;)

How many parameters are registered during a shogun run? Let's say 100 is realistic. Then we have 100 byte usage if we store only the offset, and 2.5kB if we use the masks. In a data heavy environment, and since we won't register many many more parameters this doesn't matter. For 1000 parameters it is still ok, 10^6 will result in some pretty big differences.

Lets typedef the actual type so we can change later.

karlnapf · 2018-11-09T12:38:14Z

src/shogun/base/AnyParameter.h

-				m_mask_attribute |= GRADIENT_PARAM;
-			if (hyperparameter)
-				m_mask_attribute |= HYPERPARAMETER;
+			m_attribute_mask = (hyperparameter << 0 & HYPER) |


I was more thinking
(1 << HYPER) & model_selection | (1 << GRADIENT) && gradient etc

Oh that's too low-level. Can we introduce a constexpr function for that?

yeah actually good point! :)

src/shogun/base/AnyParameter.h

karlnapf · 2018-11-12T13:21:03Z

Any news here? :)

gf712 · 2018-11-12T13:42:18Z

I did another round of clean up, but I haven't pushed it yet. Got a bit distracted looking into constexpr, as I have never used them before in my own code.. As for using a typedef I am not sure how it would be used as we are storing data in a int mask..?
Also if it is too level couldn't we use a structwith booleans instead? It would be a bit less space efficient, but would be quite easy to expand in the future?

karlnapf · 2018-11-12T17:36:10Z

I googled a bit and found this, maybe interesting?
http://blog.bitwigglers.org/using-enum-classes-as-type-safe-bitmasks/
A bit less old school ...

gf712 · 2018-11-13T11:11:01Z

So do you think we could just copy this? https://www.justsoftwaresolutions.co.uk/files/bitmask_operators.hpp
It seems to be the original code of that post and seems to cover most operations of interest.. Otherwise he wrote this more complete example.. https://github.com/Dalzhim/ArticleEnumClass-v2

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

Yeah ok, I think this is cool (I think the first simple version is fine)

We do explicitly store the mask (1,2,4,8,16, etc), rather than the offset. Easy explicit usage AnyParameterProperties::MODEL | AnyParameterProperties::HYPER
we make this typesafe through the code snippet

gf712 · 2018-11-13T20:21:06Z

Is it worth overloading the enum operators in a separate header file or does it make sense to keep it all in AnyParameter.h?

gf712 · 2018-11-13T20:22:33Z

Is it worth overloading the enum operators in a separate header file or does it make sense to keep it all in AnyParameter.h?

karlnapf · 2018-11-13T20:49:38Z

As it is general purpose code, I would copy it to lib or so

karlnapf · 2018-11-13T20:50:06Z

E.g. lib/bitmasking.h

gf712 · 2018-11-14T09:45:46Z

Not too happy with:

m_attribute_mask = ParameterProperties();
if (hyperparameter)
    m_attribute_mask |= ParameterProperties::HYPER;
if (gradient)
    m_attribute_mask |= ParameterProperties::GRADIENT;
if (model)
    m_attribute_mask |= ParameterProperties::MODEL;

but given the restrictions that are imposed in bitmask_operators.h it seems to me to be the best solution..

karlnapf · 2018-11-15T09:48:47Z

This comes from the fact that since it is now typesafe, you cannot & the mask with a boolean anymore?
It is not a big issue as we want to get rid of those booleans eventually anyways

karlnapf · 2018-11-15T09:49:44Z

src/shogun/lib/bitmask_operators.h

+// LIABLE FOR ANY DAMAGES OR OTHER LIABILITY, WHETHER IN
+// CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+// CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.


You could put "modified by G.F" here if you want

Ah ok! I wasn't sure how it works with the boost software license!

karlnapf · 2018-11-15T09:51:25Z

src/shogun/base/SGObject.h

-			!AnyParameterProperties::MODEL);
+			ParameterProperties::HYPER |
+			ParameterProperties::GRADIENT |
+            ParameterProperties::MODEL);


the whitespaces seem weird here

Yup, and should change it to default to false instead of all true.

karlnapf

LGTM!

karlnapf · 2018-11-15T09:54:03Z

Ok, we might have to touch this again at some point, but I think we can move ahead now

karlnapf · 2018-11-15T09:59:17Z

Shall we have a little test with a minimal mock class where we register some parameters with certain properties and make sure everything makes sense?

Next step would be: Let's observe every model parameter when put is called.

gf712 · 2018-11-15T10:06:33Z

This comes from the fact that since it is now typesafe, you cannot & the mask with a boolean anymore?

Yes, and adding bool to the allowed types would defeat the point of using enum class.

* combined parameter flags in a single mask * added mask_attribute to keep track of parameter availabilities * updated constructors and getters * added some documentation * changed properties default * typesafe bitmasking * refactored code to use enum class and bitmask operators

Gil added 3 commits November 7, 2018 14:20

combined parameter flags in a single mask

0a8628d

* added mask_attribute to keep track of parameter availabilities * updated constructors and getters * added some documentation

changed properties default

c505c2a

`AnyParameterProperties::HYPERPARAMETER | AnyParameterProperties::GRADIENT_PARAM | AnyParameterProperties::MODELSELECTION_PARAM` is equivalent to all parameter property flags being false (`0x00000000`)

changed modelselection_param to model_param

3aba095

modelselection_param is actually hyperparameter now and model_param is a seperate definition, e.g. bias and weights

lisitsyn reviewed Nov 7, 2018

View reviewed changes

karlnapf reviewed Nov 7, 2018

View reviewed changes

karlnapf reviewed Nov 8, 2018

View reviewed changes

karlnapf requested changes Nov 8, 2018

View reviewed changes

Gil added 2 commits November 9, 2018 10:56

fixed docs

476d2a9

switched to bitshifts instead of hexadecimals

3e32f1c

* also shorted class member names * changed default constructor to give default description

karlnapf reviewed Nov 9, 2018

View reviewed changes

src/shogun/base/AnyParameter.h Outdated Show resolved Hide resolved

Gil added 2 commits November 14, 2018 08:46

typesafe bitmasking

dfb5aea

refactored code to use enum class and bitmask operators

2bf79c4

karlnapf reviewed Nov 15, 2018

View reviewed changes

karlnapf approved these changes Nov 15, 2018

View reviewed changes

karlnapf merged commit b45243c into shogun-toolbox:develop Nov 15, 2018

gf712 deleted the parameter_refactor branch November 15, 2018 11:11

avramidis mentioned this pull request Dec 17, 2018

Removed some uses of m_model_selection_parameters [WIP] #4440

Closed

AnyParameterProperties refactor #4412

AnyParameterProperties refactor #4412

Conversation

gf712 commented Nov 7, 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

gf712 Nov 8, 2018 • edited

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

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 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

gf712 Nov 9, 2018 • edited

Choose a reason for hiding this comment

gf712 Nov 9, 2018 • edited

Choose a reason for hiding this comment

gf712 Nov 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gf712 Nov 9, 2018 • edited

Choose a reason for hiding this comment

karlnapf Nov 9, 2018 • edited

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 12, 2018

gf712 commented Nov 12, 2018 • edited

karlnapf commented Nov 12, 2018 • edited

gf712 commented Nov 13, 2018

karlnapf commented Nov 13, 2018

gf712 commented Nov 13, 2018

gf712 commented Nov 13, 2018

karlnapf commented Nov 13, 2018

karlnapf commented Nov 13, 2018

gf712 commented Nov 14, 2018

karlnapf commented Nov 15, 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

karlnapf left a comment

Choose a reason for hiding this comment

karlnapf commented Nov 15, 2018

gf712 Nov 8, 2018 •

edited

gf712 Nov 9, 2018 •

edited

gf712 Nov 9, 2018 •

edited

gf712 Nov 9, 2018 •

edited

gf712 Nov 9, 2018 •

edited

karlnapf Nov 9, 2018 •

edited

gf712 commented Nov 12, 2018 •

edited

karlnapf commented Nov 12, 2018 •

edited

gf712 commented Nov 15, 2018 •

edited