AnyParameterProperties tests #4414

gf712 · 2018-11-15T14:38:49Z

Added tests to check binary logic of AnyParameterProperties.m_attribute_mask.

Refactored all the opeations tests. * Decreased tolerance for float type tests to work with `float32_t`, `float64_t` and `floatmax_t` * Changed tests where `int` types are required to work with both `int` and `float` * Did not add `complex128_t`, `bool` or `char` tests

I added the SE test whilst there is no fix for the typed test for this case (see my PR description)

* get_epsilon is called inside each test case to determine the tolerance * tested these values with my own machine * solver tests will have a lower threshold, as these seem to change quite a bit depending on machine and lapack/blas installations * if typed testing is rolled out for all unit tests should move this to its own header file

* solvers have a fixed threshold * also fixed indentation

* avoids using several getters

gf712 · 2018-11-15T14:40:10Z

Sorry for the commit spam, not sure what happened, but they don't affect the diff.

merge

karlnapf · 2018-11-15T16:28:34Z

you need to rebase against the develop branch

gf712 · 2018-11-15T16:31:01Z

yup, I tried it earlier and it didn't work. Might be fine now though, if you want I can start a fresh PR to avoid having all those old commits?

karlnapf

great, these sanity checks are definitely useful!
Some minor comments only and then we can merge it

karlnapf · 2018-11-15T16:28:58Z

src/shogun/base/AnyParameter.h

@@ -118,6 +118,9 @@ namespace shogun
 			return static_cast<bool>(
 			    m_attribute_mask & ParameterProperties::MODEL);
 		}
+		bool is_property(ParameterProperties mask) const {


I would name it has_property

tests/unit/lib/Any_unittest.cc

karlnapf · 2018-11-15T16:31:41Z

tests/unit/lib/Any_unittest.cc

+	AnyParameterProperties params = AnyParameterProperties();
+
+	EXPECT_EQ(params.get_description(), "No description given");
+	EXPECT_FALSE(params.is_property(


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

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.

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

I would just write a test that ensures that the default parameter properties are 0

karlnapf · 2018-11-15T16:32:00Z

tests/unit/lib/Any_unittest.cc

+	    "test", EModelSelectionAvailability::MS_NOT_AVAILABLE,
+	    EGradientAvailability::GRADIENT_AVAILABLE, true);
+
+	EXPECT_EQ(params.get_description(), "test");


yep here we can/should test

karlnapf · 2018-11-15T16:33:46Z

yup, I tried it earlier and it didn't work. Might be fine now though, if you want I can start a fresh PR to avoid having all those old commits?

you can just checkout the develop branch and then git cherry-pick your local commit hash, then force push to your PR

* avoids using several getters

…meters_tests

karlnapf · 2018-11-16T11:23:41Z

tests/unit/lib/Any_unittest.cc

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


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?

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

The issue is that if you do params.has_property(ParameterProperties::DEFAULT) you end up with 0 & 0 which is 0/false.

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

karlnapf · 2018-11-16T11:27:10Z

tests/unit/lib/Any_unittest.cc

+	EXPECT_FALSE(params.has_property(
+	    ParameterProperties::HYPER & ParameterProperties::GRADIENT));
+	EXPECT_TRUE(params.has_property(
+	    ~ParameterProperties::HYPER & ParameterProperties::GRADIENT));


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)

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

karlnapf · 2018-11-16T11:27:17Z

tests/unit/lib/Any_unittest.cc

+	EXPECT_FALSE(params.has_property(
+	    ParameterProperties::HYPER & ParameterProperties::GRADIENT));
+	EXPECT_TRUE(params.has_property(
+	    ~ParameterProperties::HYPER & ParameterProperties::GRADIENT));


karlnapf

almost there :)

karlnapf · 2018-11-16T15:27:43Z

tests/unit/lib/Any_unittest.cc

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


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

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?

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.

karlnapf · 2018-11-16T15:28:12Z

tests/unit/lib/Any_unittest.cc

+{
+	AnyParameterProperties params = AnyParameterProperties();
+
+	EXPECT_TRUE(params.equals(ParameterProperties()));


yes, this does it.
And then the next 3 checks can go

oh, and also, actually it could be operator== instead of equals, or?

mmh couldnt this even be added to bitmask_operators so that == is available for all bitmasks that are defined using this?

but wouldn't that imply that all attributes would be equal, i.e. matching descriptions?

No I mean put an operator= in the enum class, the one defined via enableEnumClassBitmask(ParameterProperties);, not the AnyParameterProperties. Or doesnt that make sense?

but should still have the equals method in AnyParameterProperties that then uses the operator==, no?

Not sure ... Why would we want to have that?

Because you still need a method to access it no? Unless you want to have a getter for m_attribute_mask in AnyParameterProperties and then compare the returned value in the code to something. Wouldn't the equals method be cleaner?

see my other comment

(I agree we need a method), but equals not maybe not the best name.
What about a getter instead? Then the caller can just use ==

karlnapf · 2018-11-16T15:29:39Z

tests/unit/lib/Any_unittest.cc

+	EXPECT_EQ(params.get_gradient(), EGradientAvailability::GRADIENT_AVAILABLE);
+	EXPECT_TRUE(params.get_model());
+
+	EXPECT_TRUE(params.has_property(ParameterProperties::MODEL));


I think here we should check that the mask only has exactly the two (mode, gradient) set, but nothing else (without saying what the "else" is)
Rather than explicitly testing that bits are not set (again think about adding properties and we would have to extend the test)

So just check:

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

Yes, that would do it.

gf712 · 2018-11-16T15:51:39Z

Somewhat unrelated but should I also update the license in this file to the new short version?

karlnapf · 2018-11-16T19:47:01Z

Somewhat unrelated but should I also update the license in this file to the new short version?

yes good idea.

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

gf712 · 2018-11-22T09:41:46Z

Closing this and opened a new PR with the same commits (without the old irrelevant commits).

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

force push didnt work? Because you could have done that definitely.

gf712 · 2018-11-26T10:08:52Z

Yes, I tried https://stackoverflow.com/questions/44958129/git-new-pull-request-from-the-same-branch-still-shows-previous-commits, but then when I forced pushed it didn't work, must have messed something up somewhere..

karlnapf · 2018-11-26T11:50:15Z

next time ping me on irc and we can do it together :)

Gil added 10 commits October 15, 2018 15:48

added original SE test

00dcc2f

I added the SE test whilst there is no fix for the typed test for this case (see my PR description)

added get_epsilon to tests

b1fb379

* solvers have a fixed threshold * also fixed indentation

template formatting

02298ad

automatic tolerance detection

d94653f

moved code to TYPED_TEST

64668b5

Merge remote-tracking branch 'origin/develop' into develop

462492a

added method to check mask

1188b86

* avoids using several getters

added tests for AnyParameterProperties

fdb2d01

Merge pull request #1 from shogun-toolbox/develop

28d5d6e

merge

karlnapf requested changes Nov 15, 2018

View reviewed changes

Gil added 5 commits November 16, 2018 09:38

added method to check mask

cd7aa44

* avoids using several getters

added tests for AnyParameterProperties

e202eb9

Merge remote-tracking branch 'gf712/anyparameters_tests' into anypara…

5e2255d

…meters_tests

changed method name to has_property

896f0a8

tests refactor

caf1798

karlnapf reviewed Nov 16, 2018

View reviewed changes

karlnapf requested changes Nov 16, 2018

View reviewed changes

added equals method

722e46c

karlnapf reviewed Nov 16, 2018

View reviewed changes

overloaded == enum bitmask operator

b601b90

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

gf712 closed this Nov 22, 2018

gf712 deleted the anyparameters_tests branch December 13, 2018 15:06

AnyParameterProperties tests #4414

AnyParameterProperties tests #4414

Conversation

gf712 commented Nov 15, 2018

gf712 commented Nov 15, 2018

karlnapf commented Nov 15, 2018

gf712 commented Nov 15, 2018

karlnapf left a 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 commented Nov 15, 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

karlnapf left a 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 Nov 16, 2018 • edited

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

gf712 commented Nov 16, 2018

karlnapf commented Nov 16, 2018

gf712 commented Nov 22, 2018

karlnapf commented Nov 22, 2018

gf712 commented Nov 26, 2018

karlnapf commented Nov 26, 2018

gf712 Nov 16, 2018 •

edited