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

Closed
wants to merge 18 commits into from
Closed

AnyParameterProperties tests #4414

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
9bc6414
Refactored linalg operations tests
Oct 15, 2018
00dcc2f
added original SE test
Oct 15, 2018
e296c3f
added get epsilon function template
Oct 19, 2018
b1fb379
added get_epsilon to tests
Oct 19, 2018
02298ad
template formatting
Oct 22, 2018
d94653f
automatic tolerance detection
Nov 8, 2018
64668b5
moved code to TYPED_TEST
Nov 8, 2018
462492a
Merge remote-tracking branch 'origin/develop' into develop
Nov 15, 2018
1188b86
added method to check mask
Nov 15, 2018
fdb2d01
added tests for AnyParameterProperties
Nov 15, 2018
28d5d6e
Merge pull request #1 from shogun-toolbox/develop
Nov 15, 2018
cd7aa44
added method to check mask
Nov 15, 2018
e202eb9
added tests for AnyParameterProperties
Nov 15, 2018
5e2255d
Merge remote-tracking branch 'gf712/anyparameters_tests' into anypara…
Nov 16, 2018
896f0a8
changed method name to has_property
Nov 16, 2018
caf1798
tests refactor
Nov 16, 2018
722e46c
added equals method
Nov 16, 2018
b601b90
overloaded `==` enum bitmask operator
Nov 16, 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
3 changes: 3 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,9 @@ namespace shogun
return static_cast<bool>(
m_attribute_mask & ParameterProperties::MODEL);
}
bool has_property(ParameterProperties mask) const {
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
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
64 changes: 64 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,67 @@ 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());
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_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, putting them explicitly means we have to update the test when new properties arrive.
Why not define ParameterProperties::DEFAULT = 0x0 and then assert the default value is that?

Copy link
Member

Choose a reason for hiding this comment

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

For that we would need a method equals which returns true iff the masks exactly match

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 issue is that if you do params.has_property(ParameterProperties::DEFAULT) you end up with 0 & 0 which is 0/false.

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 can add a method with something like:

bool equals(ParameterProperties other) {
    return static_cast<int32_t>(m_attribute_mask) == static_cast<int32_t>(other);
}

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