-
Notifications
You must be signed in to change notification settings - Fork 399
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
Add QDepthwiseConv2D, DepthwiseConv2D, DepthwiseConv1D support #834
Conversation
I should also mention that this is only for 'io_stream` and only for Vivado and Vitis. There is not Quartus support. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me - for some reason the PyTests weren't triggered, so probably need to be triggered manually.
As a separate PR it could be worth including a Resource implementation of depth-wise multiplications and parameterise the new tests for both strategies.
@@ -57,6 +57,21 @@ void depthwise_conv_1d_buffer_cl(hls::stream<data_T> &data, hls::stream<res_T> & | |||
} | |||
} | |||
|
|||
template <class data_T, class res_T, typename CONFIG_T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sake of code brevity - this function can be directly called, instead of the switch statement on lines 112 - 122
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. The reason is of course historic, since the function on 112 -122 existed before. But I think I'll make the suggested change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should check if using it this way it get inlined or not, otherwise it costs a cycle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, I pushed the change. I expect that it would cause no difference in the produced code.
@@ -75,6 +75,22 @@ void depthwise_conv_2d_buffer_cl( | |||
} | |||
} | |||
|
|||
template <class data_T, class res_T, typename CONFIG_T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment for using this function below, like in the 1D case.
I think we should also support |
@bo3z, are there any other things needed for this PR? |
Looks good to me. Maybe worth opening an issue to keep track of the TODOs from this PR (resource strategy and multiplier != 1). |
model.add(DepthwiseConv2D(kernel_size=(3, 3), input_shape=(32, 32, 3))) | ||
model.compile() | ||
|
||
config = hls4ml.utils.config_from_keras_model(model, granularity='name', default_precision='fixed<32,12>') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the input is clipped to exactly match <16,6>, why do we need to increase the precision this way? Should only accum
be increased?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clipped for the keras input, just the hls4ml input. The input is actually all in the [0,1) range. I didn't try to optimize the precision in the model and just sought to make it irrelevant.
test/pytest/test_qkeras.py
Outdated
kernel_size=(3, 3), | ||
input_shape=(32, 32, 3), | ||
depthwise_quantizer='quantized_bits(6, 0, alpha=1)', | ||
depthwise_initializer='ones', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this test cheating a bit? All the weights will be 1s and the biases 0s, no point in using quantizers. It doesn't really test the proper handling of quantizers and the accuracy of the implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a leftover from me playing around with the initializer. Let me find better ones. For depthwise, the default is probably good ("he_normal"), so I'll move to that. The default for bias is 0. Let me see if I should change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably worth changing the bias initialiser - just to be sure the bias is not ignored in the C++ implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jmitrevs .
Did you tested with large kernel size?
For example: model.add(DepthwiseConv2D(kernel_size=(16,16), input_shape=(32, 32, 24))).
I'm facing some errors regarding parameters that are bigger than the int256 representation in the parameters.h file.
Description
There was a request to add support for
QDepthwiseConv2D
for QKeras. Previously we had Keras support forDepthwiseConv1D
andDepthwiseConv2D
but only as part of separable convolutions. This PR adds support for standaloneDepthwiseConv1D
andDepthwiseConv2D
, and alsoQDepthwiseConv2D
. Currently onlydepth_multiplier=1
is supported.Note that QKeras doesn't support
QDepthwiseConv1D
.Adding this found an issue in that arguments to activations passed as parameters to, e.g.
QDense
, such asactivation='quantized_relu(3, 0)'
, were ignored. This had the same effect asactivation='quantized_relu()'
.Type of change
Tests
Tests added to both
test_keras_api.py
andtest_qkeras.py
Checklist
pre-commit
on the files I edited or added.