Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return a function instead of calling it (in safe_eval) #16

Merged
merged 2 commits into from Feb 18, 2020

Conversation

vloncar
Copy link
Contributor

@vloncar vloncar commented Feb 7, 2020

If quantizer is a function (like binary_tanh or hard_sigmoid, used as activations), safe_eval would try to make an instance of it and fail. This was due to the change introduced in #12. We should check if quantizer is class to be instantiated before being called or a function ready to be called.

@googlebot googlebot added the cla: yes contribution license agreement signed label Feb 7, 2020
@zhuangh zhuangh self-requested a review February 8, 2020 05:41
@zhuangh zhuangh self-assigned this Feb 8, 2020
Copy link
Contributor

@zhuangh zhuangh left a comment

Choose a reason for hiding this comment

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

Have you tried

x = QConv2D(64, (3, 3), strides=(2,2), kernel_quantizer=binary_tanh, bias_quantizer=quantized_bits(4,0,1), name="conv2d_1_m", activation="hard_sigmoid")(x)
x = QActivation("hard_sigmoid")(x)

Does it work?

@@ -83,4 +83,7 @@ def safe_eval(eval_str, op_dict, *params, **kwparams): # pylint: disable=invali
if len(function_split) == 2 or args or kwargs:
return quantizer(*args, **kwargs)
else:
return quantizer()
if isinstance(quantizer, type):
Copy link
Contributor

Choose a reason for hiding this comment

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

please add comment here, something like "check the quantizer is a class"

@vloncar
Copy link
Contributor Author

vloncar commented Feb 10, 2020

x = QConv2D(64, (3, 3), strides=(2,2), kernel_quantizer=binary_tanh, bias_quantizer=quantized_bits(4,0,1), name="conv2d_1_m", activation="hard_sigmoid")(x)
x = QActivation("hard_sigmoid")(x)

I tried, but if i understand correctly, this is not how quantizers are intended to be used. The first hard_sigmoid is Keras's one, and the second one is QKeras's one and we never use this combination in our models. It would be useful to be able to use QKeras activations without using QActivation layer, but that is a separate issue (which we can look into).

@zhuangh
Copy link
Contributor

zhuangh commented Feb 10, 2020

Since the PR is for "If quantizer is a function (like binary_tanh or hard_sigmoid, used as activations), ...", I just want to try whether it can be used in activation inside a qkeras layers and QActivation layer directly.

@vloncar
Copy link
Contributor Author

vloncar commented Feb 10, 2020

Using QConv2d(..., activation='some_qkeras_activation'...) won't work (but we are interested in having that). Using QActivation('some_qkeras_activation'..) works for all quantizers. Combining the two doesn't make sense, i.e., anything before QAactivation should be activation=None or activation='linear'.

@zhuangh zhuangh added the ready to pull to trigger copybara-service label Feb 12, 2020
@zhuangh
Copy link
Contributor

zhuangh commented Feb 12, 2020

put this PR into internal system review.

@zhuangh zhuangh added ready to pull to trigger copybara-service and removed ready to pull to trigger copybara-service labels Feb 13, 2020
@vloncar
Copy link
Contributor Author

vloncar commented Feb 18, 2020

Is there anything else I should do for this PR?

@zhuangh
Copy link
Contributor

zhuangh commented Feb 18, 2020

sorry about the late. no for now. I am waiting for feedback from another googler (required by the internal flow).

qkeras-robot pushed a commit that referenced this pull request Feb 18, 2020
PiperOrigin-RevId: 295783821
Change-Id: I0c541eaabe348ca5d9422be18d3877ef1206ce6d
@qkeras-robot qkeras-robot merged commit ea7011b into google:master Feb 18, 2020
jecorona97 pushed a commit to jecorona97/qkeras that referenced this pull request Jun 9, 2020
PiperOrigin-RevId: 295783821
Change-Id: I0c541eaabe348ca5d9422be18d3877ef1206ce6d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes contribution license agreement signed ready to pull to trigger copybara-service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants