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

Closed
wants to merge 18 commits into from
Closed
3 changes: 3 additions & 0 deletions src/shogun/base/AnyParameter.h
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,9 @@ namespace shogun
return static_cast<bool>(
m_attribute_mask & ParameterProperties::MODEL);
}
bool is_property(ParameterProperties mask) 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 would name it has_property

return static_cast<bool>(m_attribute_mask & mask);
}

private:
std::string m_description;
Expand Down
2 changes: 2 additions & 0 deletions src/shogun/lib/bitmask_operators.h
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
66 changes: 66 additions & 0 deletions tests/unit/lib/Any_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -691,3 +691,69 @@ 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_description(), "No description given");
EXPECT_EQ(
params.get_model_selection(),
EModelSelectionAvailability::MS_NOT_AVAILABLE);
EXPECT_EQ(
params.get_gradient(), EGradientAvailability::GRADIENT_NOT_AVAILABLE);
EXPECT_FALSE(params.get_model());
Copy link
Member

Choose a reason for hiding this comment

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

get model is not part of the old API or? Just realising this

Copy link
Member Author

Choose a reason for hiding this comment

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

It isn't, but the constructor of the old API still sets the ParameterProperties::MODEL bit, so I just thought it would be appropriate to add this check?

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 this would be covered in the test below where we check that nothing is set after construction.
The old api test is more to make sure we didnt change the behaviour of the code, so remove it here.

}

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

EXPECT_EQ(params.get_description(), "No description given");
gf712 marked this conversation as resolved.
Show resolved Hide resolved
EXPECT_FALSE(params.is_property(
Copy link
Member

Choose a reason for hiding this comment

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

imagine we get more properties ... this test then needs to be touched which makes maintenance noisy.
What about just asserting that no parameters are set. i.e. the mask is zero everywhere

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! So you mean have three tests, one for each parameter? The only thing that feels like is missing is an enum for zero, since we can't compare the enum class to an int, i.e. 0.

Copy link
Member Author

Choose a reason for hiding this comment

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

well I guess 0 and ParameterProperties() are the same, so never mind!

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 just write a test that ensures that the default parameter properties are 0

ParameterProperties::MODEL | ParameterProperties::GRADIENT |
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");
Copy link
Member

Choose a reason for hiding this comment

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

yep here we can/should 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.is_property(
ParameterProperties::MODEL | ParameterProperties::GRADIENT));
EXPECT_FALSE(params.is_property(
ParameterProperties::HYPER & ParameterProperties::GRADIENT));
EXPECT_TRUE(params.is_property(
~ParameterProperties::HYPER & ParameterProperties::GRADIENT));
Copy link
Member

Choose a reason for hiding this comment

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

this doesnt make sense. You cannot check that a property is not set using has_property -- it is AND'ed with the given properties so if both are zero the and is still zero. Need the mentioned is_property (or something 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.

yup, I found it a bit odd when I wrote it. It seems unnecessarily convoluted...

EXPECT_FALSE(params.is_property(ParameterProperties::HYPER));
}

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.is_property(
ParameterProperties::MODEL | ParameterProperties::GRADIENT));
EXPECT_FALSE(params.is_property(
ParameterProperties::HYPER & ParameterProperties::GRADIENT));
EXPECT_TRUE(params.is_property(
~ParameterProperties::HYPER & ParameterProperties::GRADIENT));
Copy link
Member

Choose a reason for hiding this comment

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

as above

EXPECT_FALSE(params.is_property(ParameterProperties::HYPER));
}