AnyParameterProperties tests #4421

gf712 · 2018-11-22T09:40:49Z

Added tests to check binary logic of AnyParameterProperties.m_attribute_mask.

* avoids using several getters

this ensures that the underlying type is used to cast and make the comparisson

karlnapf · 2018-11-22T22:28:46Z

src/shogun/base/AnyParameter.h

+			return static_cast<bool>(m_attribute_mask & other_params);
+		}
+
+		bool equals(ParameterProperties other_params) const


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?

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()); }

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.

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!

karlnapf · 2018-11-22T22:29:34Z

tests/unit/lib/Any_unittest.cc

+	AnyParameterProperties params = AnyParameterProperties();
+
+	EXPECT_TRUE(params.equals(ParameterProperties()));
+	EXPECT_FALSE(params.has_property(ParameterProperties::MODEL));


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

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

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

karlnapf · 2018-11-22T22:30:12Z

tests/unit/lib/Any_unittest.cc

+	EXPECT_TRUE(params.has_property(ParameterProperties::GRADIENT));
+	EXPECT_TRUE(params.equals(
+	    ParameterProperties::MODEL | ParameterProperties::GRADIENT));
+    EXPECT_FALSE(params.equals(ParameterProperties::MODEL));


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

karlnapf · 2018-11-22T22:30:43Z

tests/unit/lib/Any_unittest.cc

+	EXPECT_TRUE(params.has_property(ParameterProperties::GRADIENT));
+	EXPECT_TRUE(params.equals(
+	    ParameterProperties::MODEL | ParameterProperties::GRADIENT));
+	EXPECT_FALSE(params.has_property(ParameterProperties::HYPER));


these last 3 as well

karlnapf · 2018-11-22T22:30:58Z

tests/unit/lib/Any_unittest.cc

+	EXPECT_TRUE(params.has_property(ParameterProperties::GRADIENT));
+	EXPECT_TRUE(params.equals(
+	    ParameterProperties::MODEL | ParameterProperties::GRADIENT));
+    EXPECT_FALSE(params.equals(ParameterProperties::MODEL));


minor indentation glitch

karlnapf · 2018-11-22T22:31:16Z

src/shogun/lib/bitmask_operators.h

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


karlnapf

just a few minor things.

* makes method names clearer

gf712 · 2018-11-30T11:27:23Z

Is this what you mean with check that none are set?

karlnapf · 2018-11-30T11:51:35Z

tests/unit/lib/Any_unittest.cc

-	EXPECT_TRUE(params.has_property(ParameterProperties::GRADIENT));
-	EXPECT_TRUE(params.equals(
-	    ParameterProperties::MODEL | ParameterProperties::GRADIENT));
+	EXPECT_FALSE(params.compare_mask(


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

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

OK! And the EXPECT_FALSE after, are they not necessary then? They should be covered by the empty mask.

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

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.

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

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.

yes that is even more explicit

This is the equivalent of zero

karlnapf · 2018-12-03T16:09:35Z

tests/unit/lib/Any_unittest.cc

+	EXPECT_FALSE(params.get_model());
+
+	EXPECT_TRUE(params.compare_mask(ParameterProperties::NONE));
+	EXPECT_FALSE(params.has_property(ParameterProperties::HYPER));


the last three are nop now

gf712 · 2018-12-12T19:00:05Z

Do you think this is ready to be merged? Started working on the new getters in CSGObject using the enums as filters, and was wondering what new methods from this PR I could use.

karlnapf · 2018-12-12T19:02:46Z

As usual, let's keep an eye on azure and buildbot

karlnapf · 2018-12-12T19:02:55Z

Thanks!

* added method to check mask * avoids using several getters * added tests for AnyParameterProperties * added equals method * overloaded `==` enum bitmask operator * Added None enum type to ParameterProperties

Gil added 6 commits November 21, 2018 17:47

added method to check mask

400fdca

* avoids using several getters

added tests for AnyParameterProperties

af9ba26

changed method name to has_property

fcfe78d

tests refactor

690c833

added equals method

27fe165

overloaded == enum bitmask operator

1e58b69

this ensures that the underlying type is used to cast and make the comparisson

karlnapf reviewed Nov 22, 2018

View reviewed changes

karlnapf requested changes Nov 22, 2018

View reviewed changes

Gil added 2 commits November 27, 2018 15:54

renamed methods

eee7074

* makes method names clearer

changed tests to check for false properties

2cc3835

karlnapf reviewed Nov 30, 2018

View reviewed changes

Added None enum type to ParameterProperties

a5dc6b5

This is the equivalent of zero

karlnapf reviewed Dec 3, 2018

View reviewed changes

removed superfluous tests

45caae8

karlnapf merged commit c6557e6 into shogun-toolbox:develop Dec 12, 2018

gf712 deleted the gf712/anyparameters_tests branch December 13, 2018 15:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AnyParameterProperties tests #4421

AnyParameterProperties tests #4421

gf712 commented Nov 22, 2018

karlnapf Nov 22, 2018

gf712 Nov 26, 2018

karlnapf Nov 26, 2018

gf712 Nov 26, 2018

karlnapf Nov 22, 2018

gf712 Nov 26, 2018

karlnapf Nov 26, 2018

karlnapf Nov 22, 2018

karlnapf Nov 22, 2018

karlnapf Dec 3, 2018

karlnapf Nov 22, 2018

karlnapf Nov 22, 2018

karlnapf left a comment

gf712 commented Nov 30, 2018

karlnapf Nov 30, 2018

karlnapf Nov 30, 2018

gf712 Nov 30, 2018

karlnapf Nov 30, 2018

gf712 Nov 30, 2018

karlnapf Dec 1, 2018

gf712 Dec 3, 2018

karlnapf Dec 3, 2018

karlnapf Dec 3, 2018

gf712 commented Dec 12, 2018

karlnapf commented Dec 12, 2018

karlnapf commented Dec 12, 2018

AnyParameterProperties tests #4421

AnyParameterProperties tests #4421

Conversation

gf712 commented Nov 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karlnapf left a comment

Choose a reason for hiding this comment

gf712 commented Nov 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gf712 commented Dec 12, 2018

karlnapf commented Dec 12, 2018

karlnapf commented Dec 12, 2018