Conform to PEP8 standards #421

danthedaniel · 2017-04-22T02:03:49Z

What does this PR do?

Provides Python code that (for the most part) conforms to PEP8 standards.
- Line length wasn't taken too seriously, although I still tried to stick with 80 characters when reasonable
- The code copied from DEAP wasn't treated as strictly as the rest of the code
Improves code readability through functional decomposition

coveralls · 2017-04-22T02:06:59Z

Coverage increased (+0.007%) to 77.392% when pulling 0b9ea71 on teaearlgraycold:code_cleanup into 591c13e on rhiever:development.

coveralls · 2017-04-22T02:19:30Z

Coverage increased (+0.007%) to 77.392% when pulling 2b8636d on teaearlgraycold:code_cleanup into 591c13e on rhiever:development.

rhiever · 2017-04-24T19:08:06Z

This will take a while to review, but let's try to get this merged ASAP so it doesn't slow down other development efforts. @weixuanfu2016, can you please take a look in the next day or two to see if you find any issues? I will plan to do so as well.

weixuanfu · 2017-04-24T19:13:19Z

OK, will do

weixuanfu · 2017-04-24T19:24:34Z

tests.py

-
+    """Assert that the TPOTClassifier score function outputs a known score for a fix pipeline."""
    tpot_obj = TPOTClassifier()
    known_score = 0.977777777778  # Assumes use of the TPOT balanced_accuracy function


May need to delete or update this comment since the default scoring function is accuracy instead of balanced_accuracy

weixuanfu · 2017-04-24T19:33:07Z

tpot/driver.py

-        tpot_type = TPOTClassifier
-    else:
-        tpot_type = TPOTRegressor
+def _impute_missing_values(features, missing_value):


I suggest that this imputation function could also be added into interactive mode by moving it to base.py

danthedaniel · 2017-04-24T19:49:18Z

Oops. the imputation function was mistakenly left in. I'll remove that and update the PR.

weixuanfu · 2017-04-24T19:44:50Z

tpot/driver.py


-    if args.VERBOSITY in [1, 2] and tpot._optimized_pipeline:
-        training_score = max([tpot._pareto_front.keys[x].wvalues[1] for x in range(len(tpot._pareto_front.keys))])
+    if args.VERBOSITY < 3 and tpot._optimized_pipeline:


Here need to be [1, 2] for no stdout when verbosity=0

weixuanfu · 2017-04-24T19:46:57Z

tpot/gp_deap.py

            ind_str = str(ind1)
            num_loop = 0
-            while ind_str == str(ind1) and num_loop < 50 : # 50 loops at most to generate a different individual by crossover
+            while ind_str == str(ind1) and num_loop < 50:  # 50 loops at most to generate a different individual by crossover


May also use NUM_TESTS instead as decorators.py

Sure, might as well get rid of the magic numbers from DEAP.

coveralls · 2017-04-24T22:38:23Z

Coverage increased (+0.4%) to 77.778% when pulling e864671 on teaearlgraycold:code_cleanup into 591c13e on rhiever:development.

rhiever · 2017-04-25T18:35:57Z

tests.py

+
+
+# http://stackoverflow.com/questions/5595425/
+def is_close(a, b, rel_tol=1e-09, abs_tol=0.0):


We should use numpy.allclose here: https://docs.scipy.org/doc/numpy/reference/generated/numpy.allclose.html

rhiever · 2017-04-25T19:18:06Z

tpot/base.py

+            testing_features.astype(np.float64),
+            testing_classes.astype(np.float64)
+        )
+        return abs(score)


Is returning abs of the score the correct thing to do here? The blame graph shows that we added it when we added regressors, but what is the thinking behind it?

IIRC some scorers produce scores in the range [0, ∞), but the regressors can have scores in the range (-∞, ∞).

OK. I'll file a separate issue for this topic since it's not particularly related to this PR.

rhiever · 2017-04-25T19:25:17Z

tpot/built_in_operators.py


    @property
    def __name__(self):
+        """Instance ame is the same as the class name."""


Typo here: ame

rhiever · 2017-04-25T19:26:29Z

tpot/config_classifier.py


    'sklearn.feature_selection.SelectKBest': {
-        'k': range(1, 100), # need check range!
+        'k': range(1, 100),  # TODO: Check range


@weixuanfu2016, please file an issue for this TODO.

rhiever · 2017-04-25T19:36:31Z

OK. After the two issues I mentioned above are settled, I'm 👍 on merging.

coveralls · 2017-04-25T19:38:45Z

Coverage increased (+0.4%) to 77.778% when pulling b5842c5 on teaearlgraycold:code_cleanup into 591c13e on rhiever:development.

coveralls · 2017-04-25T20:49:13Z

Coverage increased (+0.5%) to 77.884% when pulling cb2b4a5 on teaearlgraycold:code_cleanup into 591c13e on rhiever:development.

danthedaniel · 2017-04-25T20:51:34Z

Hold on, ignore that commit. I'm going to remove something and rebase.

rhiever · 2017-04-25T20:55:31Z

tpot/base.py

-                        file_string = input_file.read()
-                    self.config_dict = eval(file_string[file_string.find('{'):(file_string.rfind('}') + 1)])
+                    config_module = import_module(config_dict)
+                    self.config_dict = config_module.tpot_config


Was this change intended? I thought this was a separate PR.

See my comment above

Hopefully that doesn't change too much. :-) My only gripe is this new functionality here.

coveralls · 2017-04-25T21:03:06Z

Coverage increased (+0.4%) to 77.778% when pulling e67714d on teaearlgraycold:code_cleanup into 591c13e on rhiever:development.

rhiever · 2017-04-25T21:06:26Z

PR looks good to merge. Any outstanding issue before I merge it?

danthedaniel · 2017-04-25T21:07:39Z

Don't think so. Merge away

Conform to PEP8 standards

0b9ea71

Use assert_raises in tests

2b8636d

rhiever self-requested a review April 24, 2017 19:06

rhiever added the enhancement label Apr 24, 2017

weixuanfu reviewed Apr 24, 2017

View reviewed changes

rhiever reviewed Apr 25, 2017

View reviewed changes

tpot/built_in_operators.py Outdated

@property

def __name__(self):

"""Instance ame is the same as the class name."""

Copy link

rhiever Apr 25, 2017

Choose a reason for hiding this comment

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

Typo here: ame

rhiever reviewed Apr 25, 2017

View reviewed changes

Use np.allclose instead of is_close function in tests

e67714d

rhiever merged commit affa041 into EpistasisLab:development Apr 25, 2017

AIAdventures mentioned this pull request Jun 6, 2017

Titanic example -problem with 2nd last cell. #492

Closed



		# http://stackoverflow.com/questions/5595425/
		def is_close(a, b, rel_tol=1e-09, abs_tol=0.0):

Conform to PEP8 standards #421

Conform to PEP8 standards #421

Uh oh!

Conversation

danthedaniel commented Apr 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Uh oh!

coveralls commented Apr 22, 2017

Uh oh!

coveralls commented Apr 22, 2017

Uh oh!

rhiever commented Apr 24, 2017

Uh oh!

weixuanfu commented Apr 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danthedaniel commented Apr 24, 2017 via email • edited by rhiever Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weixuanfu Apr 24, 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!

coveralls commented Apr 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danthedaniel Apr 25, 2017 • edited by rhiever 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!

rhiever commented Apr 25, 2017

Uh oh!

coveralls commented Apr 25, 2017

Uh oh!

coveralls commented Apr 25, 2017

Uh oh!

danthedaniel commented Apr 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhiever Apr 25, 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!

coveralls commented Apr 25, 2017

Uh oh!

rhiever commented Apr 25, 2017

Uh oh!

danthedaniel commented Apr 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

danthedaniel commented Apr 22, 2017 •

edited

Loading

danthedaniel commented Apr 24, 2017 via email •

edited by rhiever

Loading

weixuanfu Apr 24, 2017 •

edited

Loading

danthedaniel Apr 25, 2017 •

edited by rhiever

Loading

rhiever Apr 25, 2017 •

edited

Loading