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.

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

AnyParameterProperties tests #4421

Merged
merged 10 commits into from
Dec 12, 2018
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 21 additions & 21 deletions tests/unit/lib/Any_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -708,48 +708,48 @@ TEST(AnyParameterProperties, new_api_default)
{
AnyParameterProperties params = AnyParameterProperties();

EXPECT_TRUE(params.equals(ParameterProperties()));
EXPECT_FALSE(params.has_property(ParameterProperties::MODEL));
EXPECT_FALSE(params.has_property(ParameterProperties::GRADIENT));
EXPECT_FALSE(params.has_property(ParameterProperties::HYPER));
EXPECT_TRUE(params.compare_mask(ParameterProperties()));
}

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

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));
EXPECT_EQ(
params.get_gradient(), EGradientAvailability::GRADIENT_NOT_AVAILABLE);
EXPECT_FALSE(params.get_model());

EXPECT_FALSE(params.compare_mask(
ParameterProperties::MODEL | ParameterProperties::HYPER |
ParameterProperties::HYPER));
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

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

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

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_EQ(
params.get_gradient(), EGradientAvailability::GRADIENT_NOT_AVAILABLE);
EXPECT_FALSE(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.compare_mask(
Copy link
Member

Choose a reason for hiding this comment

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

nope, this just checks that one is not set.
you want to check that none are set. So you need to create an "empty" mask (all zeros) and then call compare_mask

Copy link
Member

Choose a reason for hiding this comment

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

but this empty mask should be created via not explicitly listing the available types.
Again, this is to avoid having to touch all these tests in the case when a type is added

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! And the EXPECT_FALSE after, are they not necessary then? They should be covered by the empty mask.

Copy link
Member

Choose a reason for hiding this comment

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

not sure what you mean, but what about something like
EXPECT_TRUE(params.compare_mask(ParameterProperties::EMPTY)) or similar

Copy link
Member Author

Choose a reason for hiding this comment

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

But EXPECT_TRUE(params.compare_mask(ParameterProperties::EMPTY)) is the same as EXPECT_TRUE(params.compare_mask(ParameterProperties())). I can add a EMPTY though to the enum.

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, and of course it is the same. But then it wouldnt test that the default properties are indeed empty, so the test would not be of an use: It would just test that if you create two properties with the default ctor, they have the same parameters. If you would check for EMPTY (or maybe we call it NONE (and define it to be 0x0), then you also assert that the default ctor generates empty parameters, which is exactly what we want here. If people then later start messing around with the ctor (e.g. change the default), the test will fail, so it provides some protection against devs going wild

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 ok! But then in that case it might make more sense to instantiate the m_attribute_mask class member in the constructors with ParameterProperties::EMPTY rather than ParameterProperties() no? I just used ParameterProperties() because it was the easiest way to represent zero without any casting.

Copy link
Member

Choose a reason for hiding this comment

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

yes that is even more explicit

ParameterProperties::MODEL | ParameterProperties::HYPER |
ParameterProperties::HYPER));
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

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