Typecheck forbidden p3 #16

smohsinali · 2017-01-13T13:11:40Z

No description provided.

mfeurer

I added some minor comments. Could you please work on them and also fix the conflict to the master branch?

mfeurer · 2017-01-16T08:37:53Z

ConfigSpace/forbidden.py

 class AbstractForbiddenComponent(object):
    __metaclass__ = ABCMeta
-
+    # todo : should absract methods be annotated with types?


From my point of view, this is not necessary here. Did mypy complain? If yes, we have to do something about it. Else, please remove the comment. The other abstract methods which should return something should be annotated though.

no mypy didnt complain...removed this todo

mfeurer · 2017-01-16T08:46:11Z

ConfigSpace/forbidden.py


 class AbstractForbiddenClause(AbstractForbiddenComponent):
-    def get_descendant_literal_clauses(self):
+    def get_descendant_literal_clauses(self) -> List['AbstractForbiddenClause']:


Wouldn't AbstractForbiddenComponent be more appropriate her? Also, why is this a string? Moreover, the definition further below suggests that this function return List[Hyperparameter].

changed.
also its different class so no need to declare it as str.
why string? http://stackoverflow.com/questions/33533148/how-do-i-specify-that-the-return-type-of-a-method-is-the-same-as-the-class-itsel

mfeurer · 2017-01-16T08:48:03Z

ConfigSpace/forbidden.py


 class SingleValueForbiddenClause(AbstractForbiddenClause):
-    def __init__(self, hyperparameter, value):
+    # todo: possible types of hyperparameter valuees?


I'm afraid we need to accept Any here. Please remove the comment.

mfeurer · 2017-01-16T08:50:09Z

ConfigSpace/forbidden.py

        self.value = value

-    def is_forbidden(self, instantiated_hyperparameters, strict=True):
+    def is_forbidden(self, instantiated_hyperparameters: Hyperparameter, strict: bool=True) -> bool:


Please check the code by running it, but I think that instantiated_hyperparameters is a dict. Please also annotate _is_forbidden.

changed the type of instantiated_hyperparameters to Dict[str, Union[None, str, float, int]] .
all versions of _is_forbidden are annotated except abstract methods

mfeurer · 2017-01-16T08:50:54Z

ConfigSpace/forbidden.py


 class MultipleValueForbiddenClause(AbstractForbiddenClause):
-    def __init__(self, hyperparameter, values):
+    # todo: type of vals of hp


Same as above.

mfeurer · 2017-01-16T08:51:12Z

ConfigSpace/forbidden.py

 class MultipleValueForbiddenClause(AbstractForbiddenClause):
-    def __init__(self, hyperparameter, values):
+    # todo: type of vals of hp
+    def __init__(self, hyperparameter: Hyperparameter, values: Any) -> None:


Same as above.

mfeurer · 2017-01-16T08:52:26Z

ConfigSpace/forbidden.py

        pass

-    def get_descendant_literal_clauses(self):
+    def get_descendant_literal_clauses(self) -> List[Hyperparameter]:


Please update this based on the comment above.

mypy fails if set to AbstractForbiddenComponent or AbstractForbiddenConjunction at following lines:
dlcs = self.get_descendant_literal_clauses()
for dlc in dlcs:
if dlc.hyperparameter.name not in ihp_names:

mfeurer · 2017-01-16T08:52:34Z

ConfigSpace/forbidden.py

        return children

-    def is_forbidden(self, instantiated_hyperparameters, strict=True):
+    def is_forbidden(self, instantiated_hyperparameters: Hyperparameter, strict: bool=True) -> bool:


Same as above.

mfeurer · 2017-01-16T08:53:13Z

ConfigSpace/forbidden.py

        for component in self.components:
-            e = component.is_forbidden(instantiated_hyperparameters,
-                                       strict=strict)
+            e = component.is_forbidden(instantiated_hyperparameters)


type of strict is bool so its value should be 'true' or 'false' and not 'strict'

Yes, but strict is a boolean variable that is passed to is_forbidden in line 195; so it won't change till down here.

…ypecheck_forbidden_p3 # Conflicts: # ConfigSpace/hyperparameters.py

mfeurer

Two more things to do, then we can merge this.

mfeurer · 2017-01-20T08:31:13Z

ConfigSpace/forbidden.py

        pass
-
-    def get_descendant_literal_clauses(self):
+    # todo: recheck is return type should be AbstractForbiddenComponent or AbstractForbiddenConjunction or Hyperparameter


Could you please check this with the debugger?

mypy accepts only List[hyperparameter] but according to debugger AbstractForbiddenComponent should also work

What do you mean by also? Can both happen?

from output of debugger it seems that the object of type forbidden has hyperparameter as its keys

fixed by adding hyperparameter attribute to AbstractForbiddenComponent

mfeurer · 2017-01-20T08:32:25Z

ConfigSpace/forbidden.py

        for component in self.components:
-            e = component.is_forbidden(instantiated_hyperparameters,
-                                       strict=strict)
+            e = component.is_forbidden(instantiated_hyperparameters)


Yes, but strict is a boolean variable that is passed to is_forbidden in line 195; so it won't change till down here.

…ypecheck_hyperparameter_p3 # Conflicts: # ConfigSpace/hyperparameters.py

coveralls · 2017-01-20T11:40:52Z

Coverage increased (+0.02%) to 67.359% when pulling 18ea83f on smohsinali:typecheck_forbidden_p3 into a7f2c84 on automl:master.

coveralls · 2017-01-25T04:35:55Z

Coverage increased (+0.03%) to 67.369% when pulling 2a4c1db on smohsinali:typecheck_forbidden_p3 into a7f2c84 on automl:master.

smohsinali added 3 commits January 13, 2017 14:10

type annotate forbidden.py

135a819

fix

3e29549

io.reduce->reduce

dec99c3

mfeurer requested changes Jan 16, 2017

View reviewed changes

smohsinali added 2 commits January 20, 2017 00:20

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

f74101c

…ypecheck_forbidden_p3 # Conflicts: # ConfigSpace/hyperparameters.py

made changes according to comments on pull request

ddc7033

mfeurer requested changes Jan 20, 2017

View reviewed changes

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

18ea83f

…ypecheck_hyperparameter_p3 # Conflicts: # ConfigSpace/hyperparameters.py

add hyperparameter attribute to abstractforbiddencomponent

2a4c1db

mfeurer approved these changes Jan 25, 2017

View reviewed changes

mfeurer merged commit 89cd0cd into automl:master Jan 25, 2017

smohsinali deleted the typecheck_forbidden_p3 branch June 8, 2017 02:12

Typecheck forbidden p3 #16

Typecheck forbidden p3 #16

Uh oh!

Conversation

smohsinali commented Jan 13, 2017

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!

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

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!

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!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

smohsinali Jan 19, 2017 •

edited

Loading

coveralls commented Jan 20, 2017 •

edited

Loading

coveralls commented Jan 25, 2017 •

edited

Loading