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.

Sign up for GitHub

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

Jump to bottom

AnyParameterProperties refactor #4412

Merged
merged 7 commits into from
Nov 15, 2018
Merged

AnyParameterProperties refactor #4412

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0a8628d
combined parameter flags in a single mask
Nov 7, 2018
c505c2a
changed properties default
Nov 7, 2018
3aba095
changed modelselection_param to model_param
Nov 7, 2018
476d2a9
fixed docs
Nov 9, 2018
3e32f1c
switched to bitshifts instead of hexadecimals
Nov 9, 2018
dfb5aea
typesafe bitmasking
Nov 14, 2018
2bf79c4
refactored code to use enum class and bitmask operators
Nov 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
81 changes: 70 additions & 11 deletions src/shogun/base/AnyParameter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
/*
* This software is distributed under BSD 3-clause license (see LICENSE file).
*
* Authors: Heiko Strathmann, Gil Hoben
*/

#ifndef __ANYPARAMETER_H__
#define __ANYPARAMETER_H__

Expand All @@ -21,49 +27,102 @@ namespace shogun
GRADIENT_NOT_AVAILABLE = 0,
GRADIENT_AVAILABLE = 1
};

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

OK!

* 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
* vectors and bias
*/
class AnyParameterProperties
{
public:
AnyParameterProperties()
: m_description(), m_model_selection(MS_NOT_AVAILABLE),
m_gradient(GRADIENT_NOT_AVAILABLE)
static const int32_t HYPERPARAMETER = 0x00000001;
Copy link
Member

Choose a reason for hiding this comment

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

What about making it an int enum?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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 it is pretty clear since the class is ending with "Parameter", so everything in there is one ....

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

fine as well

static const int32_t GRADIENT_PARAM = 0x00000010;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@gf712 gf712 Nov 8, 2018 •
edited

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

I would call this GRADIENT_AVAILABLE

static const int32_t MODEL_PARAM = 0x00000100;

/** default constructor where all parameter types are switched to false
Copy link
Member

Choose a reason for hiding this comment

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

Minor: sentences start with capital letters

*/
AnyParameterProperties() : m_description(), m_mask_attribute(0x00000000)
{
}
/** legacy constructor */
Copy link
Member

Choose a reason for hiding this comment

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

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

AnyParameterProperties(
std::string description,
EModelSelectionAvailability model_selection = MS_NOT_AVAILABLE,
EGradientAvailability gradient = GRADIENT_NOT_AVAILABLE)
EGradientAvailability gradient = GRADIENT_NOT_AVAILABLE,
bool hyperparameter = false)
: m_description(description), m_model_selection(model_selection),
m_gradient(gradient)
{
m_mask_attribute = 0x00000000;
if (model_selection)
Copy link
Member

Choose a reason for hiding this comment

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

wouldnt this work?

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

Copy link
Member

Choose a reason for hiding this comment

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

or in the new style

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

m_mask_attribute |= MODEL_PARAM;
if (gradient)
m_mask_attribute |= GRADIENT_PARAM;
if (hyperparameter)
m_mask_attribute |= HYPERPARAMETER;
}
/** mask constructor */
Copy link
Member

Choose a reason for hiding this comment

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

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

AnyParameterProperties(std::string description, int32_t mask_attribute)
: m_description(description)
{
m_mask_attribute = mask_attribute;
}
/** copy contructor */
AnyParameterProperties(const AnyParameterProperties& other)
: m_description(other.m_description),
m_model_selection(other.m_model_selection),
m_gradient(other.m_gradient)
m_gradient(other.m_gradient),
m_mask_attribute(other.m_mask_attribute)
{
}

/** description getter */
Copy link
Member

Choose a reason for hiding this comment

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

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

std::string get_description() const
Copy link
Member

Choose a reason for hiding this comment

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

minor what about returning a const string reference

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

{
return m_description;
}

/** model selection flag getter */
Copy link
Member

Choose a reason for hiding this comment

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

same here, would remove the comment

EModelSelectionAvailability get_model_selection() const
{
return m_model_selection;
EModelSelectionAvailability return_val;
if (m_mask_attribute & HYPERPARAMETER)
return_val = EModelSelectionAvailability::MS_AVAILABLE;
else
return_val = EModelSelectionAvailability::MS_NOT_AVAILABLE;
return return_val;
}

/** gradient flag getter */
EGradientAvailability get_gradient() const
{
return m_gradient;
EGradientAvailability return_val;
if (m_mask_attribute & GRADIENT_PARAM)
return_val = EGradientAvailability::GRADIENT_AVAILABLE;
Copy link
Member

Choose a reason for hiding this comment

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

it makes sense to return immediately without return_val

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean with a one liner like this?

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

Copy link
Member

Choose a reason for hiding this comment

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

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

else
return_val = EGradientAvailability::GRADIENT_NOT_AVAILABLE;
return return_val;
}

/** hyperparameter flag getter */
bool get_hyperparameter() const
{
bool return_val;
if (m_mask_attribute & HYPERPARAMETER)
return_val = true;
else
return_val = false;
return return_val;
}

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

Choose a reason for hiding this comment

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

what about calling it "attribute_mask" ? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

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

};

class AnyParameter
Expand Down Expand Up @@ -116,6 +175,6 @@ namespace shogun
Any m_value;
AnyParameterProperties m_properties;
};
}
} // namespace shogun

#endif
20 changes: 16 additions & 4 deletions src/shogun/base/SGObject.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Copy link
Member

Choose a reason for hiding this comment

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

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

AnyParameterProperties::GRADIENT_PARAM |
AnyParameterProperties::MODEL_PARAM))
{
BaseTag tag(name);
create_parameter(tag, AnyParameter(make_any_ref(value), properties));
Expand All @@ -694,7 +697,10 @@ class CSGObject
void watch_param(
const std::string& name, T** value, S* len,
AnyParameterProperties properties = AnyParameterProperties(
"Unknown parameter", MS_NOT_AVAILABLE, GRADIENT_NOT_AVAILABLE))
"Unknown parameter",
AnyParameterProperties::HYPERPARAMETER |
AnyParameterProperties::GRADIENT_PARAM |
AnyParameterProperties::MODEL_PARAM))
{
BaseTag tag(name);
create_parameter(
Expand All @@ -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",
Copy link
Member

Choose a reason for hiding this comment

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

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

AnyParameterProperties::HYPERPARAMETER |
AnyParameterProperties::GRADIENT_PARAM |
AnyParameterProperties::MODEL_PARAM))
{
BaseTag tag(name);
create_parameter(
Expand All @@ -733,7 +742,10 @@ class CSGObject
{
BaseTag tag(name);
AnyParameterProperties properties(
"Dynamic parameter", MS_NOT_AVAILABLE, GRADIENT_NOT_AVAILABLE);
"Dynamic parameter",
AnyParameterProperties::HYPERPARAMETER |
AnyParameterProperties::GRADIENT_PARAM |
AnyParameterProperties::MODEL_PARAM);
std::function<T()> bind_method =
std::bind(method, dynamic_cast<const S*>(this));
create_parameter(tag, AnyParameter(make_any(bind_method), properties));
Expand Down