Typecheck hyperparameter p3 #21

smohsinali · 2017-01-13T17:47:41Z

Remaing mypy errors in hyperparameters.py file: (mixin and value_dict)

hyperparameters.py:208: error: "is_legal" undefined in superclass
hyperparameters.py:211: error: "UniformMixin" has no attribute "upper"
hyperparameters.py:211: error: "UniformMixin" has no attribute "lower"
hyperparameters.py: note: In member "check_default" of class "UniformMixin":
hyperparameters.py:222: error: "check_default" undefined in superclass
hyperparameters.py: note: In member "init" of class "OrdinalHyperparameter":
hyperparameters.py:761: error: Need type annotation for variable
hyperparameters.py:764: error: Cannot determine type of 'value_dict'
hyperparameters.py: note: In member "get_order" of class "OrdinalHyperparameter":
hyperparameters.py:829: error: Cannot determine type of 'value_dict'
hyperparameters.py: note: In member "get_value" of class "OrdinalHyperparameter":
hyperparameters.py:836: error: Cannot determine type of 'value_dict'

…ypecheck_hyperparameter_p3 # Conflicts: # ConfigSpace/hyperparameters.py

…o typecheck_hyperparameter_p3 # Conflicts: # ConfigSpace/hyperparameters.py

mfeurer · 2017-01-25T20:08:27Z

I just pushed some fixes, please fix the remaining bugs. Code looks okay, but needs a little bit of restructuring and cleaning after you fixed the bugs ;)

change self.check_int to check_int

coveralls · 2017-01-26T15:49:06Z

Coverage decreased (-0.1%) to 67.228% when pulling f921ef5 on smohsinali:typecheck_hyperparameter_p3 into 4aa95af on automl:master.

mfeurer

This looks good. Comments are mostly of cosmetic nature, asking you to remove comments ;)

mfeurer · 2017-01-27T14:08:34Z

ConfigSpace/hyperparameters.py

+    if not (isinstance(value, float) or isinstance(value, int)):
+        return False
+    # Strange numerical issues!
+    # todo: discuss acceptable tolerance (without big tolerance for upper the test fails)


Tolerance is fine for now, please remove the todo and other comment.

mfeurer · 2017-01-27T14:10:09Z

ConfigSpace/hyperparameters.py

-from functools import reduce
+import numpy as np
+
+


Actually, all these methods are specific to one class, right? If yes, could you please move them to the respective classes?

both io and np are used in all classes

mfeurer · 2017-01-27T14:11:11Z

ConfigSpace/hyperparameters.py

+
 class FloatHyperparameter(NumericalHyperparameter):
-    def __init__(self, name, default):
+    # todo : type of name and default?


Yup, they're correct. Please remove the comment.

mfeurer · 2017-01-27T14:11:22Z

ConfigSpace/hyperparameters.py

-        return np.round(float(default), 10)
+    @abstractmethod
+    def is_legal(self, value: Union[int, float]) -> bool:
+        # return isinstance(value, float) or isinstance(value, int)


Please remove old return statements.

mfeurer · 2017-01-27T14:11:37Z

ConfigSpace/hyperparameters.py

+        # return isinstance(value, float) or isinstance(value, int)
+        raise NotImplemented

+    # todo : recheck default


Type correct. Please remove comment.

mfeurer · 2017-01-27T14:19:42Z

ConfigSpace/hyperparameters.py

-
-    def _transform(self, vector):
+
+    # todo recheck return type...is it list or normal


I don't get this comment.

outdated comment...removed

mfeurer · 2017-01-27T14:20:13Z

ConfigSpace/hyperparameters.py

-
-    def get_value(self, idx):
+
+    # todo : recheck


correct, please remove comment.

mfeurer · 2017-01-27T14:20:27Z

ConfigSpace/hyperparameters.py

            return False

-    def _sample(self, rs, size=None):
+    def _sample(self, rs: np.random, size: Union[int, None] = None) -> int:


same as above.

changed to np.random.RandomState

mfeurer · 2017-01-27T14:20:56Z

ConfigSpace/hyperparameters.py

            return 2

-    def get_neighbors(self, value, number=2, transform = False):
+    # todo: recehck...added rs as param otherrwise mismatch with baseclass signature


Yep, correct, although it's not used here. Please remove the comment.

mfeurer · 2017-01-27T14:21:21Z

test/test_hyperparameters.py

+        upper = 10
+        f1 = UniformFloatHyperparameter("param", lower, upper, q=0.1, log=True)
+
+        # self.assertTrue(f1.is_legal(3.0))


Why are these all commented out?

is_legal was changed to is_legal_uniformfloat but is now reverted back

coveralls · 2017-01-30T11:35:47Z

Coverage decreased (-0.03%) to 67.302% when pulling 18f5620 on smohsinali:typecheck_hyperparameter_p3 into 4aa95af on automl:master.

mfeurer

Hi, three minor things, plus, please make sure that the variable rs is actually annotated with np.random.RandomState instead of np.random.

mfeurer · 2017-01-30T20:26:07Z

ConfigSpace/hyperparameters.py

        return True

-    def get_num_neighbors(self, value: None=None) -> np.inf:
+    def get_num_neighbors(self, value= None) -> np.inf:


Why did you remove the type declaration here?

mypy doesnt accept None=None

mfeurer · 2017-01-30T20:27:31Z

ConfigSpace/hyperparameters.py

-
-    def check_default(self, default: int) -> int:
+    @abstractmethod
+    def check_default(self, default) -> int:


No type definition here.

this is abstract method. in its definition in UniformIntegerHyperparameter type of default is 'Union[int, float]' while in NormalIntegerHyperparameter type of default is 'int' so because of this conflict i removed it form abstract method.

if you want in some different way i can do that for example making it same and then in init of NormalIntegerHyperparameter doing a check to make sure that its only 'int'

mfeurer · 2017-01-30T20:30:13Z

ConfigSpace/hyperparameters.py


    # todo check if conversion should be done in initiation call or inside class itsel
-    def to_uniform(self, z: int = 3) -> 'UniformIntegerHyperparameter':
+    def to_uniform(self, z: np.round = 3) -> 'UniformIntegerHyperparameter':


I didn't mean the type should be np.round, I meant you should use np.round(int(self.mu - (z * self.sigma))) instead of only int(self.mu - (z * self.sigma)) to make sure that the hyperparameter is rounded correctly to the next int.

coveralls · 2017-01-31T02:08:39Z

Coverage decreased (-0.03%) to 67.302% when pulling d06086c on smohsinali:typecheck_hyperparameter_p3 into 4aa95af on automl:master.

smohsinali and others added 22 commits January 13, 2017 14:46

type annotate hyperparameter.py

430a758

.

7449e5d

commit

3ce095a

commit

04f4bb9

annotation of hyperparameters.py

098ae65

fix return type np.NaN to float

ff277b7

UniformIntegerHyperparameter -> 'UniformIntegerHyperparameter'

64dc681

np.NaN -> float

cd58555

fix

0d538da

np.randint -> np.random

0e8bb3d

io.reduce->reduce

1b4e373

create class baseuniformfloathyparameter

332f5f8

Merge branch 'master' into typecheck_hyperparameter_p3

2a5c41e

Merge branch 'master' of https://github.com/automl/ConfigSpace into t…

7c105a5

…ypecheck_hyperparameter_p3 # Conflicts: # ConfigSpace/hyperparameters.py

Merge branch 'master' of https://github.com/automl/ConfigSpace into t…

4003448

…ypecheck_hyperparameter_p3 # Conflicts: # ConfigSpace/hyperparameters.py

make changes according to comments on pull request

1c12347

is_leagal and check_default functions

1677cc8

Merge remote-tracking branch 'origin/typecheck_hyperparameter_p3' int…

7e91533

…o typecheck_hyperparameter_p3 # Conflicts: # ConfigSpace/hyperparameters.py

is_leagal and check_default functions

3b1306a

is_leagal and check_default functions

d14a9e1

FIX some bugs introduced in previous restructuring

aea2259

Merge branch 'master' into smohsinali-typecheck_hyperparameter_p3

b9127ea

change tolerance for floats

f921ef5

change self.check_int to check_int

mfeurer requested changes Jan 27, 2017

View reviewed changes

smohsinali added 2 commits January 30, 2017 12:09

put is_legal and check_defalut functions to specific classes

2840683

put is_legal and check_defalut functions to specific classes

18f5620

mfeurer requested changes Jan 30, 2017

View reviewed changes

rs.random to rs.random.randomstate

d06086c

mfeurer approved these changes Jan 31, 2017

View reviewed changes

mfeurer merged commit 1943118 into automl:master Jan 31, 2017


		def _transform(self, vector):

		# todo recheck return type...is it list or normal

Typecheck hyperparameter p3 #21

Typecheck hyperparameter p3 #21

Uh oh!

Conversation

smohsinali commented Jan 13, 2017

Uh oh!

mfeurer commented Jan 25, 2017

Uh oh!

coveralls commented Jan 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mfeurer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jan 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mfeurer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jan 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

coveralls commented Jan 26, 2017 •

edited

Loading

coveralls commented Jan 30, 2017 •

edited

Loading

coveralls commented Jan 31, 2017 •

edited

Loading