Repository navigation
Conversation
The QKeras parser reads the kernel, recurrent and bias quantizers of QSimpleRNN, QLSTM and QGRU into layer attributes, but the recurrent layers added their weights without them, so hls4ml wrote the float weights into the weight type while QKeras rounds them. Pass the quantizers when the weights are added, as Dense does. The Altera LSTM initializer, which splits the weights per gate, now uses the recurrent and bias quantizers for the recurrent kernel and the bias instead of the kernel quantizer for all three. Tests: the QKeras recurrent tests check that the weights equal the QKeras-quantized values and run on Vivado and Vitis too; a new test compares QSimpleRNN, QLSTM and QGRU with 8-bit and power-of-two weights against QKeras on both backends. Fixes fastmachinelearning#1579
|
Thanks for this PR. This is what Claude Opus 5.5 found: Review: #1598 — Apply QKeras weight quantizers to SimpleRNN, LSTM and GRUVerdict: request changes. The fix itself is correct. The blocking issue is in the tests: the new weight check makes the Quartus and Altera recurrent tests fail, and the tests can't tell the three quantizers apart. Tested with: qkeras-v3 (Keras 3.14.1, TF 2.21) on the PR branch. I did not run What's right
Blocking: the weight check fails on Quartus and Altera
Once the transpose is accounted for, the values are correct on both backends, so the problem is the test, not the fix. One option is to pass the backend into the helper: def assert_rnn_weights_quantized(hls_layer, quantizers, backend):
for name, q in quantizers.items():
w = hls_layer.weights[name]
expected = np.asarray(q(w.data_unquantized))
if backend in ('Quartus', 'Altera') and expected.ndim == 2:
expected = expected.T
np.testing.assert_array_equal(w.data, expected)Test coverageThe tests can't distinguish the quantizersThe existing tests use the same quantizer for kernel, recurrent and bias, and the new As a mutation test, I swapped To catch that kind of mix-up:
The Altera change itself is untestedA conversion-only test needs no toolchain. It could convert a QLSTM with distinct quantizers and check
The Resource strategy isn't coveredThe zero padding in #1589's weight layout interacts badly with po2 weights. To reproduce on Vivado: q = dict(kernel_quantizer='quantized_po2(4)', recurrent_quantizer='quantized_po2(4)',
bias_quantizer='quantized_bits(8, 0, alpha=1)')
x = Input((6, 4))
model = Model(x, QLSTM(8, activation='tanh', recurrent_activation='sigmoid', **q)(x))
config = hls4ml.utils.config_from_keras_model(model, granularity='name', backend='Vivado',
default_precision='ap_fixed<24,10>')
name = model.layers[1].name
config['LayerName'][name]['Strategy'] = 'Resource'
config['LayerName'][name]['ReuseFactor'] = 3 # n_cycles=3, 2 input lanes -> 6 rows for 4 inputsThe layout pads the 4 input rows to 6 with zeros, and
It still compiles, and the outputs match QKeras (max difference 0.021 for QLSTM, 0.031 for QGRU) because the kernel skips padded lanes ( I'd suggest:
Smaller pointsQuartusLeaving Quartus for #1537 is reasonable, but #1537 hasn't merged yet. Things I observed:
Double quantization in Altera
|
The Keras 3 converter has no handler for the qkeras-v3 recurrent layers, so they go through the Keras 2 fallback, which appended a separate Activation layer for the layer's activation. The recurrent layer applies its activation itself, so tanh was applied twice to the output of QLSTM and QGRU (relu is idempotent, so QSimpleRNN was unaffected). Skip the extra layer for recurrent layers, as the Keras 2 converter does.
…two weights Review follow-up for the QKeras recurrent quantizers. The tests use a different quantizer for the kernel, the recurrent kernel and the bias (8, 6 and 4 bits, or 4- and 5-bit power-of-two with an 8-bit bias), so a quantizer applied to the wrong weight is caught; GRU also checks the recurrent bias. The weight check compares against the quantized unquantized data and transposes the expectation on Quartus and Altera, which store the kernels transposed, so the three backend tests now check the weights on all four backends instead of skipping the check there. A conversion-only test checks the per-gate Altera LSTM weights and their types; the whole-array weights are quantized before the Altera initializer runs, so the per-gate pieces are now cut from the unquantized data and quantized once, as before. The Resource strategy of Vivado and Vitis zero-pads the recurrent kernels, and the power-of-two formatter took log2 of those zeros, writing the minimum int64 as the exponent. Zeros now get the smallest exponent of the type. The new test runs the Latency and Resource strategies, checks that every exponent is within the type, and compares the outputs against QKeras.
Thanks for your reply @JanFSchulte ! I ran some tests with Fable 5.1 to address the issues, reviewed the results and it seems to be fixed now, here's what it found: Weight check on Quartus / Altera — fixed with the helper you suggested: it compares against Distinct quantizers — the three tests use 8-bit kernel / 6-bit recurrent / 4-bit bias ( Altera — new Resource strategy / Keras 3 (found while doing the above) — the qkeras-v3 recurrent layers have no v3 handler and go through Quartus coarse-kernel compile failure and Bidirectional — agreed those are pre-existing; both are now listed under "Related, not changed here" in the description. I can open issues for them. Runs on this branch: |
Description
parse_qrnn_layerreads the kernel, recurrent and bias quantizers ofQSimpleRNN,QLSTMandQGRUinto theweight_quantizer,recurrent_weight_quantizerandbias_quantizerattributes, butSimpleRNN,LSTMandGRUinhls4ml/model/layers.pyadded their weights without them. hls4ml therefore truncated the float weights to the weight type while QKeras rounds them, and the two computed with weights that differ by up to one step of the type. The layers now pass the quantizers when they add their weights, asDensedoes. For layers that do not come from QKeras the attributes areNoneand nothing changes.The Altera backend's
init_lstmsplits the LSTM weights per gate and quantizes the pieces itself. It usedweight_quantizerfor the kernel, the recurrent kernel and the bias; it now usesrecurrent_weight_quantizerandbias_quantizerfor the last two, and cuts the pieces fromdata_unquantized, so they are quantized once (the whole arrays are already quantized wheninit_lstmruns). This only matters when the three quantizers differ. The Quartus backend has the same code and is left alone because #1537 removes it.The Resource strategy of Vivado and Vitis (#1589) zero-pads the recurrent kernels to whole lanes. With power-of-two weights
ExponentWeightVariable._formattooklog2(0)of the padding and wrote-9223372036854775808as the exponent (with numpy warnings); it now gives zeros the smallest exponent of the type,-2**(width-1)for a signed type. The kernel skips the padded lanes, so the outputs do not change.The Keras 3 converter has no handler for the qkeras-v3 recurrent layers; they go through the Keras 2 fallback in
KerasV3HandlerDispatcher.v2_call, which appended a separateActivationlayer for the layer'sactivation. The recurrent layer applies its activation itself, sotanhwas applied twice to QLSTM and QGRU outputs (reluis idempotent, so QSimpleRNN was unaffected). The fallback now skips the extra layer for recurrent layers, as the Keras 2 converter does. This is in its own commit and can be split off.With the script from #1579 (8-bit
quantized_bits(8, 0, alpha=1)weights, Vivado,ap_fixed<24,10>), the largest difference from QKeras:relu)Vitis gives the same numbers. Power-of-two and binary kernels were unusable before. On the test model below (weights drawn from (-0.5, 0.5), 50 inputs, Vivado) with
quantized_po2(4)kernels and an 8-bit bias, the difference from QKeras on main is 0.45 / 0.22 / 0.48 for QSimpleRNN / QLSTM / QGRU, with output ranges of 0.68 / 0.56 / 1.27, because hls4ml wrote the float weights into the exponent type; with this PR it is 0.0005 / 0.0102 / 0.0096. Withbinary(alpha=1)kernels and an 8-bit bias, main gives 310 / 0.50 / 1.38 (the output range); with this PR the weights match QKeras' (checked), and the outputs differ from QKeras by up to 0.0000 / 0.023 / 0.041. All of these are the same with both QKeras packages. The LSTM and GRU residuals with binary kernels are larger than with 8-bit ones, which I have not looked into (the weights are right, so it is in the kernels' handling of ±1 weights, which this PR does not touch); binary is not in the test below.The remaining difference is hls4ml's
tanh/sigmoidlookup tables: for every layer and both QKeras packages, the QKeras output equals a plain Keras layer loaded with the quantized weights to 4 decimals, and hls4ml differs from that float reference by the amounts in the table. The main column differs between the two packages because of the double activation in the Keras 3 converter described above; with that fixed the two packages give identical outputs.Fixes #1579
Related, not changed here
ternary()andbinary()with their defaultalpha='auto'/'auto_po2'on recurrent layers: QKeras scales each output column by its own factor, and hls4ml'sQKerasFactorizeAlphapass moves those scales into anApplyAlphalayer only for Dense and convolution layers. For recurrent layers the scaled values go into a 1- or 2-bit integer type, so the result is wrong on main and still wrong with this PR (QKeras vs hls4ml differences equal the output range). Supporting that needs the pass extended to recurrent layers; I can open an issue.nnet_dense_latency.h:49: no matching function for call to 'ap_fixed<24, 10>::ap_fixed(config2_1::bias_t&)', same on main). The recurrent layers now hit it the same way as Dense.init_lstmstill quantizes every per-gate piece with the kernel quantizer and gives it the kernel's type. With a finer kernel quantizer the values are right (re-quantizing already-quantized weights on a finer grid is a no-op); a coarser kernel quantizer than the bias (3-bit kernel, 8-bit bias) fails to compile becausennet::lstmtakes all biases asCONFIG_T::bias_t. Same code on main, and Retire Quartus backend #1537 removes the backend.Bidirectional: the parser acceptsQLSTM/QGRUsublayers but never reads their quantizers, andBidirectional(QLSTM)crashes on Keras 3 (Bidirectional.initializechecks for the class nameLSTM, soQLSTMtakes the GRU branch). Pre-existing; not touched here.Type of change
Affected areas
Backends:
init_lstmquantizer choice and slicing)hls4ml.model:layers.py,types.py)Frontends:
Components:
Configurations affected and exercised
io_typetest_qsimplernn,test_qlstm,test_qgru, weight check + compile), QKeras 0.9 and qkeras-v3compile()needs the Altera toolchain, which I do not haveImpact on generated HLS
Numerical behaviour: intentionally changed for QKeras recurrent layers only: the weight arrays written into the project are now the QKeras-rounded values instead of the float weights. The weight types are unchanged:
config_from_keras_modelalready took them from the quantizers (fixed<8,1>for the layers above), only the values written into them differ. Models without QKeras recurrent layers are bit-exact vs main. With power-of-two weights under the Resource strategy the padded exponents are now the type's minimum instead of the int64 minimum. I did not run synthesis; the kernels are unchanged, only the weight values.Tests
In
test/pytest/test_qkeras.pyandtest/pytest/test_qkerasV3.py:test_qsimplernn,test_qlstmandtest_qgrunow also run on Vivado and Vitis (QSimpleRNN needs Fix recurrent layers on Vivado and Vitis, and add SimpleRNN #1577, merged), use a different quantizer per weight (8-bit kernel, 6-bit recurrent kernel, 4-bit bias), and check on all four backends that the layer's kernel, recurrent kernel and bias (and GRU's recurrent bias) equal the QKeras-quantized values of the model's weights; the helper transposes the expectation on Quartus and Altera, which store the kernels transposed. Intest_qkeras.pytheQSimpleRNN,QLSTMandQGRUentries ofNO_QUANT_LAYERSrun on all backends too.test_qrnn_weight_quantizers, parametrized over the three layers, Vivado / Vitis, Latency / Resource (ReuseFactor=3), and two quantizer sets,quantized_bits8 / 6 / 4 bits andquantized_po2(4)/quantized_po2(5)/ 8-bit bias: the model from QKeras recurrent layers (QSimpleRNN, QLSTM, QGRU): weight quantizers are not applied, so results differ from QKeras (Keras 2 and Keras 3) #1579 with 50 inputs, the weight check (Latency only, since the Resource layout reorders the data), a check that every written power-of-two exponent is within the type, and a comparison with QKeras atatol=0.05.test_qlstm_altera_gate_quantizers(conversion only, no toolchain): a QLSTM with 8 / 4 / 3-bit quantizers on Altera; checks the whole-array weights and each gate'sweight_g,recurrent_weight_gandbias_gagainst the QKeras-quantized slices, and their typesfixed<8,1>,fixed<4,1>,fixed<3,1>.atol=0.1in the three tests is kept. In their configuration (ap_fixed<8,1>for everything, including accumulators and results, one input) the difference is dominated by the 8-bit arithmetic, not by the weights: measured over three seeds on Vivado and Vitis, QLSTM is 0.025–0.035 on main and the same with this PR, QGRU 0.029–0.045 on main and 0.028–0.037 with this PR. A tolerance that fails main and passes the fix does not exist there, so the weight check carries the regression test in those three and the new test carries the numerical one.On
mainwith these test files,test_qsimplernn,test_qlstmandtest_qgruon Vivado, Vitis and Quartus,test_qrnn_weight_quantizersandtest_qlstm_altera_gate_quantizersall fail (34 failures per file, at the weight check). On this branch:The Quartus variants of the three modified tests and the Altera conversion test pass in both environments; the Altera variants of the three modified tests fail at
compile()withCould not find ahls, as onmain.QKeras 0.9 with TensorFlow 2.14 fails inside its recurrent layers with
AttributeError: module 'tensorflow.python.util.nest' has no attribute 'is_sequence'(andarray_ops.unstack), as noted in #1579. The local runs above put those two functions back through a pytest plugin outside the repository; nothing in this PR works around it.Test configuration: Linux, Python 3.10.20 with TensorFlow 2.14.1 / Keras 2.14.0 / QKeras 0.9.0, and Python 3.11 with TensorFlow 2.21.0 / Keras 3.14.1 / qkeras-v3 1.2.1. No HLS tool.
AI assistance disclosure
Tool(s) and model(s): Claude Code (Claude Fable 5.1)
Where it was used: the layer and Altera changes (as proposed in #1579), the
types.pyand Keras 3 converter changes, the test changes, the measurement scripts behind the numbers above, and the drafts of this description and of the review replies.Checklist
pre-commiton the files I edited.test/pytestcovering this change (a bug fix should add a test that fails onmainand passes here).Release note