Class config #21

rhsimplex · 2017-06-13T13:23:32Z

@josh for discussion - minimal delta for the configuration change w.r.t. #10

codecov-io · 2017-06-13T13:27:11Z

Codecov Report

Merging #21 into master will decrease coverage by 2.82%.
The diff coverage is 91.19%.

@@           Coverage Diff           @@
##           master   #21      +/-   ##
=======================================
- Coverage   92.82%   90%   -2.83%     
=======================================
  Files          11    12       +1     
  Lines         516   500      -16     
=======================================
- Hits          479   450      -29     
- Misses         37    50      +13

jwayne · 2017-06-13T14:26:47Z

This looks great to me. I just made some small modifications (~15min) to get this branch running on the following examples. I've manually tested that all visualizations work for these examples.

FLASK_APP=picasso PICASSO_SETTINGS=examples/keras/config.py flask run
FLASK_APP=picasso PICASSO_SETTINGS=examples/tensorflow/config.py flask run

Let's review this now! And I'd say, let's not worry about documentation or the codecov report for now - we can fix that in separate PR's that we'll merge into this branch.

rhsimplex · 2017-06-14T09:20:10Z

tests/test_picasso.py

-class TestTensorflowBackend:
+from picasso.ml_frameworks.tensorflow.model import TFModel
+class TFTestModel(TFModel):
+    TF_PREDICT_VAR = 'Softmax:0'


~~this overrides the configuration file~~

rhsimplex · 2017-06-14T09:42:53Z

picasso/examples/tensorflow/model.py

+
+class TensorflowMNISTModel(TFModel):
+
+    TF_INPUT_VAR = 'convolution2d_input_1:0'


this is configuration and shouldn't be overridden here

jwayne · 2017-06-14T22:57:15Z

@rhsimplex I saw you made a few updates:

In KerasModel, convert self.tf_predict_var, self.tf_input_var to lower case
In TFModel, convert class attributes TF_PREDICT_VAR, TF_INPUT_VAR to lower-case. Use
these class attributes' values unless the arguments tf_predict_var, tf_input_var are passed into the constructor, in which we override them with the arguments' values.

I agree with (1) because these attributes are set in an instance method (specifically, the instance constructor), which makes them class attributes. Thus, they should be lower case. Furthermore, in TFModel, these are lower-case attributes, and these attributes should be named the same way in TFModel and KerasModel. (The notion that these attributes should have the same name in TFModel and KerasModel will be clearer when we convert tf_predict_var, tf_input_var to properties in a shared interface)

I disagree with (2) for 2 reasons. Aside from breaking the convention that class attributes be upper-case, more importantly, it is bad style to override class attributes in an instance. Class attributes belong to the class, so generally users expect that they are untouched by an instance. Instances should only modify instance attributes, which they own. In this particular case, I'd recommend converting the class attributes' names back to upper case and then defining the TFModel constructor as follows:

def __init__(self, *args, **kwargs):
    super(TFModel, self).__init__(*args, **kwargs)
    if not hasattr(self, 'tf_predict_var'):
        self.tf_predict_var = self.TF_PREDICT_VAR
    if not hasattr(self, 'tf_input_var'):
        self.tf_input_var = self.TF_INPUT_VAR

Now, this is still bad style, since generally we shouldn't be using hasattr checks. But we can fix this in the separate formatting/styling PR.

jan-xyz · 2017-06-15T06:57:16Z

@jwayne, I'm assuming you are using PEP8 code style in picasso.

According to PEP8 all caps is reserved for constants not for class variables. Class variables should lead with a double under score (e.g. __tf_predict_value) and they can be accessed by calling the base class directly since python doesn't import variables with that naming scheme automatically (e.g. baseModel.__tf_predict_model).

here the reference to the section in PEP8: https://www.python.org/dev/peps/pep-0008/#method-names-and-instance-variables

another question: why are you not specifying the parameters for the models? In my opinion using *args and **kwargs obfuscates and reduces readability. Furthermore you could shorten the entire logic by using default parameters directly:

def __init__(self, 
             tf_predict_var=BaseModel.__tf_predit_var,
             tf_input_var=BaseModel.__tf_input_var):
    super(TFModel, self).__init__(*args, **kwargs)
    self.tf_predict_var = tf_predict_var
    self.tf_input_var = tf_input_var

rhsimplex · 2017-06-15T08:07:16Z

@jwayne @jan-xyz I agree the casing is not correct! But since I'm trying for the minimal delta for the current feature, and they are upper case and lower case throughout the code, I just changed them all to the same case to make the test pass (since the visualizations refer to the lower-case versions).

We're doing another PR with style changes on top of this one, we can fix it there if that's alright (and even merge it in to this one before merging to master).

EDIT @jwayne haha i see you said the same thing in the last sentence of your post. I should read more.

jwayne · 2017-06-15T08:56:44Z

@rhsimplex Ok let's fix it in the other pr then. It was unclear to me whether we wanted new code in this pr to follow style guides or not. To me that made sense but if we're fixing it in the next pr anyway then I don't care since the result is the same.

…

On Thu, Jun 15, 2017, 10:07 AM Ryan Henderson ***@***.***> wrote: @jwayne <https://github.com/jwayne> @jan-xyz <https://github.com/jan-xyz> I agree the casing is not correct! But since I'm trying for the minimal delta for the current feature, and they are upper case and lower case throughout the code, I just changed them all to the same case to make the test pass (since the visualizations refer to the lower-case versions). We're doing another PR with style changes on top of this one, we can fix it there if that's alright (and even merge it in to this one before merging to master). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#21 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABPiiYvdQUP1bNXT09Eaxe7oKsnhgSEGks5sEOY1gaJpZM4N4e7L> .

jwayne · 2017-06-15T09:11:57Z

@jan-xyz yes that's what I meant, thanks. But in this case TF_PREDICT_VAR, etc are class constants. Anyway this is now a moot point since we will fix in next pr.

…

On Thu, Jun 15, 2017, 10:56 AM Josh Chen ***@***.***> wrote: @rhsimplex Ok let's fix it in the other pr then. It was unclear to me whether we wanted new code in this pr to follow style guides or not. To me that made sense but if we're fixing it in the next pr anyway then I don't care since the result is the same. On Thu, Jun 15, 2017, 10:07 AM Ryan Henderson ***@***.***> wrote: > @jwayne <https://github.com/jwayne> @jan-xyz <https://github.com/jan-xyz> > I agree the casing is not correct! But since I'm trying for the minimal > delta for the current feature, and they are upper case and lower case > throughout the code, I just changed them all to the same case to make the > test pass (since the visualizations refer to the lower-case versions). > > We're doing another PR with style changes on top of this one, we can fix > it there if that's alright (and even merge it in to this one before merging > to master). > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#21 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/ABPiiYvdQUP1bNXT09Eaxe7oKsnhgSEGks5sEOY1gaJpZM4N4e7L> > . >

rhsimplex · 2017-06-16T09:12:33Z

If it looks ok to you @jwayne, then I'll update the documentation and merge (or leave this branch separate for now, whichever you prefer).

jwayne · 2017-07-05T17:02:19Z

@rhsimplex I finished making the code style and formatting changes in the last 2 commits. I'd recommend looking at them independently. I think they should be relatively parseable, let me know and thanks!

…-config

jwayne · 2017-07-06T12:24:51Z

picasso/__init__.py

+                     ' setting.  The settings and utility functions'
+                     ' have been changed as of version v0.2.0 (and '
+                     'you\'re using {}). Changing to the updated '
+                     ' settings is trivial: see xxxxxxx.'.format(__version__))


remember to update the xxxxx

jwayne · 2017-07-06T12:24:59Z

picasso/__init__.py

+                       'BACKEND_POSTPROCESSOR_NAME',
+                       'BACKEND_POSTPROCESSOR_PATH',
+                       'BACKEND_PROB_DECODER_NAME',
+                       'BACKEND_PROB_DECODER_PATH']


probably want to add DATA_DIR here

This reverts commit 1d396da.

rhsimplex added 2 commits June 13, 2017 15:11

move configuration to classes

c5f21ec

move configuration to classes

0a58bab

get branch running

def486d

get test passing

40e311c

rhsimplex commented Jun 14, 2017

View reviewed changes

rhsimplex added 3 commits June 14, 2017 11:25

fixup model files: docstring length, function scope

b1d281e

fix config

d90fe0c

consistent attributes

ec77da8

rhsimplex commented Jun 14, 2017

View reviewed changes

rhsimplex added 4 commits June 14, 2017 11:44

don't override settings in model example

d7a9c2a

parameterize tests

371a43b

revert to lowercase attributes

91b8df8

add deprecation error

a2337b6

Merge branch 'master' into class-config

9cc02cd

jwayne force-pushed the class-config branch 3 times, most recently from c1667d6 to a7c7255 Compare July 5, 2017 13:58

Code style and formatting changes.

c2a8b58

jwayne force-pushed the class-config branch from a7c7255 to c2a8b58 Compare July 5, 2017 14:01

Rename ml_frameworks.*, visualizations.base

da65a5f

jwayne force-pushed the class-config branch from b9cb260 to da65a5f Compare July 5, 2017 22:08

jwayne and others added 3 commits July 6, 2017 10:58

remove path from description

1a17ecf

cleanup docstrings and minor formatting

88ea820

Merge branch 'class-config' of github.com:merantix/picasso into class…

f9335b4

…-config

jwayne reviewed Jul 6, 2017

View reviewed changes

rhsimplex added 3 commits July 6, 2017 17:35

restore header

9ce855f

update documentation

dc02c41

warning message for deprecated settings

33cf81e

rhsimplex merged commit 1d396da into master Jul 7, 2017

rhsimplex deleted the class-config branch July 7, 2017 09:06

rhsimplex added a commit that referenced this pull request Jul 7, 2017

Revert "Class config (#21)"

91493c2

This reverts commit 1d396da.


		class TensorflowMNISTModel(TFModel):

		TF_INPUT_VAR = 'convolution2d_input_1:0'

Class config #21

Class config #21

Uh oh!

Conversation

rhsimplex commented Jun 13, 2017

Uh oh!

codecov-io commented Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jwayne commented Jun 13, 2017

Uh oh!

rhsimplex Jun 14, 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!

rhsimplex Jun 14, 2017

Choose a reason for hiding this comment

Uh oh!

jwayne commented Jun 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jan-xyz commented Jun 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhsimplex commented Jun 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jwayne commented Jun 15, 2017 via email

Uh oh!

jwayne commented Jun 15, 2017 via email

Uh oh!

rhsimplex commented Jun 16, 2017

Uh oh!

jwayne commented Jul 5, 2017

Uh oh!

jwayne Jul 6, 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!

jwayne Jul 6, 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!

rhsimplex Jul 6, 2017

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Jun 13, 2017 •

edited

Loading

rhsimplex Jun 14, 2017 •

edited

Loading

jwayne commented Jun 14, 2017 •

edited

Loading

jan-xyz commented Jun 15, 2017 •

edited

Loading

rhsimplex commented Jun 15, 2017 •

edited

Loading

jwayne Jul 6, 2017 •

edited

Loading

jwayne Jul 6, 2017 •

edited

Loading