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 tests #4421

Merged
merged 10 commits into from
Dec 12, 2018
Merged

AnyParameterProperties tests #4421

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
400fdca
added method to check mask
Nov 15, 2018
af9ba26
added tests for AnyParameterProperties
Nov 15, 2018
fcfe78d
changed method name to has_property
Nov 16, 2018
690c833
tests refactor
Nov 16, 2018
27fe165
added equals method
Nov 16, 2018
1e58b69
overloaded `==` enum bitmask operator
Nov 16, 2018
eee7074
renamed methods
Nov 27, 2018
2cc3835
changed tests to check for false properties
Nov 30, 2018
a5dc6b5
Added None enum type to ParameterProperties
Dec 3, 2018
45caae8
removed superfluous tests
Dec 3, 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
9 changes: 9 additions & 0 deletions src/shogun/base/AnyParameter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,15 @@ namespace shogun
return static_cast<bool>(
m_attribute_mask & ParameterProperties::MODEL);
}
bool has_property(ParameterProperties other_params) const
{
return static_cast<bool>(m_attribute_mask & other_params);
}

bool equals(ParameterProperties other_params) const
Copy link
Member

Choose a reason for hiding this comment

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

i am not sure about this. equals should always just be true if really everything is semantically equal. So that would include the description and the name. Maybe a rename?

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 just think in most cases the description will differ, yet we only want to know if they have the same ParameterProperties.

Could just have:

/** Compare masks */
bool compare_mask(ParameterProperties other_params) const
{
	return m_attribute_mask == other_params ;
}

and then:

/** Compare AnyParameterProperties properties */
bool equals(AnyParameterProperties other) const
{
	return (description == other.description) && has_mask(other.get_mask());
}

Copy link
Member

Choose a reason for hiding this comment

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

first one is definitely useful, so let's have that
Second one I dont know whether we need this? But if we do, then all fields need to be compared of course.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the second one is a bit of overkill and I cant think of a situation where it would be useful.. I'll just stick to the first one!

{
return m_attribute_mask == other_params;
}

private:
std::string m_description;
Expand Down
8 changes: 8 additions & 0 deletions src/shogun/lib/bitmask_operators.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@
// CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
// CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.
//
// modified by Gil Hoben

#include<type_traits>

Expand Down Expand Up @@ -106,5 +108,11 @@ namespace shogun {
static_cast<underlying>(lhs) ^ static_cast<underlying>(rhs));
return lhs;
}
template<typename E>
Copy link
Member

Choose a reason for hiding this comment

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

++

typename std::enable_if<enable_bitmask_operators<E>::enable, bool>::type
operator==(E lhs, E rhs) {
typedef typename std::underlying_type<E>::type underlying;
return static_cast<underlying>(lhs) == static_cast<underlying>(rhs);
}
}
#endif
62 changes: 62 additions & 0 deletions tests/unit/lib/Any_unittest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -691,3 +691,65 @@ TEST(Any, lazy_cloneable_visitable)
EXPECT_FALSE(any.visitable());
EXPECT_THROW(any.visit(nullptr), std::logic_error);
}

TEST(AnyParameterProperties, old_api_default)
{
AnyParameterProperties params = AnyParameterProperties();

EXPECT_EQ(
params.get_model_selection(),
EModelSelectionAvailability::MS_NOT_AVAILABLE);
EXPECT_EQ(
params.get_gradient(), EGradientAvailability::GRADIENT_NOT_AVAILABLE);
EXPECT_FALSE(params.get_model());
}

TEST(AnyParameterProperties, new_api_default)
{
AnyParameterProperties params = AnyParameterProperties();

EXPECT_TRUE(params.equals(ParameterProperties()));
EXPECT_FALSE(params.has_property(ParameterProperties::MODEL));
Copy link
Member

Choose a reason for hiding this comment

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

as said before, can we not explicitly test that those are not set, but in fact that none are set. Otherwise we will have to modify tests when we add new attributes

Copy link
Member Author

Choose a reason for hiding this comment

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

I did think I saw that comment in the previous PR, but the next day the comment disappeared..

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that happens on force push. I keep the notification emails to not loose comments when github hides them.

EXPECT_FALSE(params.has_property(ParameterProperties::GRADIENT));
EXPECT_FALSE(params.has_property(ParameterProperties::HYPER));
}

TEST(AnyParameterProperties, old_custom_ctor)
{
AnyParameterProperties params = AnyParameterProperties(
"test", EModelSelectionAvailability::MS_NOT_AVAILABLE,
EGradientAvailability::GRADIENT_AVAILABLE, true);

EXPECT_EQ(params.get_description(), "test");
EXPECT_EQ(
params.get_model_selection(),
EModelSelectionAvailability::MS_NOT_AVAILABLE);
EXPECT_EQ(params.get_gradient(), EGradientAvailability::GRADIENT_AVAILABLE);
EXPECT_TRUE(params.get_model());

EXPECT_TRUE(params.has_property(ParameterProperties::MODEL));
EXPECT_TRUE(params.has_property(ParameterProperties::GRADIENT));
EXPECT_TRUE(params.equals(
ParameterProperties::MODEL | ParameterProperties::GRADIENT));
EXPECT_FALSE(params.equals(ParameterProperties::MODEL));
Copy link
Member

Choose a reason for hiding this comment

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

everything else should be false. And that should be tested. Not just these two, same reasoning as above

Copy link
Member

Choose a reason for hiding this comment

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

minor indentation glitch

EXPECT_FALSE(params.has_property(ParameterProperties::HYPER));
Copy link
Member

Choose a reason for hiding this comment

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

the last three are nop now

}

TEST(AnyParameterProperties, new_custom_ctor)
{
AnyParameterProperties params = AnyParameterProperties(
"test", ParameterProperties::MODEL | ParameterProperties::GRADIENT);

EXPECT_EQ(params.get_description(), "test");
EXPECT_EQ(
params.get_model_selection(),
EModelSelectionAvailability::MS_NOT_AVAILABLE);
EXPECT_EQ(params.get_gradient(), EGradientAvailability::GRADIENT_AVAILABLE);
EXPECT_TRUE(params.get_model());

EXPECT_TRUE(params.has_property(ParameterProperties::MODEL));
EXPECT_TRUE(params.has_property(ParameterProperties::GRADIENT));
EXPECT_TRUE(params.equals(
ParameterProperties::MODEL | ParameterProperties::GRADIENT));
EXPECT_FALSE(params.has_property(ParameterProperties::HYPER));
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

Copy link
Member

Choose a reason for hiding this comment

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

these last 3 as well

}