Performance on master (default accumulators precision)

[thesps]

Running through the hls4ml-tutorial using current hls4ml master branch, I see an issue in the numerical performance of the hls4ml evaluated accuracy of the QKeras model in part 4. The printout of the accuracy gives:

Accuracy baseline:  0.7502650602409638
Accuracy pruned, quantized: 0.7456385542168674
Accuracy hls4ml: 0.20196385542168674

With the most recent release, hls4ml v0.5.0, for the same model, same QKeras version, I don't see this issue. The printout of the accuracy gives:

Accuracy baseline:  0.7502650602409638
Accuracy pruned, quantized: 0.7456385542168674
Accuracy hls4ml: 0.7455481927710843

The part 1 models (regular float Keras) achieves good accuracy with hls4ml master.

I will try to dig a bit further, but others may encounter this issue.

[jmduarte]

The issue is the behavior of accum_t. Previously it was set to whatever the default model precision is (ap_fixed<16,6> in the tutorial I think), but now we set it to the same precision as the input type (I think it's ap_fixed<7,1> in part 4 of the tutorial).

This causes the accumulator to overflow.

I think neither is a good "default behavior" although the previous one did tend to just "work out of the box" more easily.

We should somehow tell the user to pick a good accum_t Precision or set it ourselves internally to a better default (at least ap_fixed<7+log2(n_acc),1+log2(n_acc)> where n_acc is the number of inputs that need to be accumulated up to some preset maximum (maybe ap_fixed<32,16>)

[zhuangh]

The issue is the behavior of accum_t. Previously it was set to whatever the default model precision is (ap_fixed<16,6> in the tutorial I think), but now we set it to the same precision as the input type (I think it's ap_fixed<7,1> in part 4 of the tutorial).

This causes the accumulator to overflow.

what is the way of assigning accumt_t in hls4ml?

I think neither is a good "default behavior" although the previous one did tend to just "work out of the box" more easily.

We should somehow tell the user to pick a good accum_t Precision or set it ourselves internally to a better default (at least ap_fixed<7+log2(n_acc),1+log2(n_acc)> where n_acc is the number of inputs that need to be accumulated up to some preset maximum (maybe ap_fixed<32,16>)

What @jmduarte mentioned is actually what we do in qkeras.qtools

[thesps]

Okay, I get it now. Yeah I agree neither using the default precision nor the input precision is necessarily best. I would say setting the accumulator precision to the same as the input precision is less likely to be correct, though. It would make more sense to set it to the same as the layer output precision (although that may not have been set optimally either...)

We could derive the accumulator precision from the input type, weight type and number of inputs like you described. I tend to think that will generally be safe, but conservative and use more bits than you strictly need. We'd have to think where to put it too, for example for models trained non-QAT, maybe we don't want to do that. The recent PR #321 should be able to set those precisions by sampling from data, although right now it only sets the layer output type, not the accumulator.

@maksgraczyk could you take a look at that? I think we didn't discuss it before, but as well as setting the layer output from the sampled data you could derive the required accumulator type given the data, which can also be set in the configuration dictionary.

Maybe the behaviour should be... have the config generation utility set the default accumulator type to the conservative one that will guarantee no overflow regardless of the data, then have the new set_data_types_from_keras_model from #321 able to set it to something narrower based on the real values.

This does then leave the question of what to do with the layer output type too. For example, if the 'wide' accumulator is used, I think we may still want to have a more narrow type for the layer output to avoid a scenario where (for example) a QDense is followed by a BatchNormalization and we send ap_fixed<32,16> data into the BatchNorm...

[maksgraczyk]

could you take a look at that? I think we didn't discuss it before, but as well as setting the layer output from the sampled data you could derive the required accumulator type given the data, which can also be set in the configuration dictionary.

Thank you for tagging me. I'm currently looking into making set_data_types_from_keras_model() set accum_t of a layer to the same type as the layer output one (e.g. if the method infers that fc1 should have result precision ap_fixed<11,2>, its accum_t will also be ap_fixed<11,2>). The implementation I'm currently testing does exactly that and it seems to resolve the QKeras performance issue.

Is there motivation for accum_t to be different than the actual layer output precision?

[thesps]

I'm currently looking into making set_data_types_from_keras_model() set accum_t of a layer to the same type as the layer output one

That sounds great 👍

Is there motivation for accum_t to be different than the actual layer output precision?

I think yes, for example you might accumulate with a wide type to avoid overflow, then reduce to something narrower to provide to the next layer. Though on this I'm not so sure... it's probably mostly safe to have accumulator and output precision set to the same.

In addition, it would be good if you can implement a new method to derive the accumulator type not from data, but to be the narrowest type that still guarantees that no overflow can happen, based on the layer type, the precision of the input(s) and the number of inputs. We can go into a bit more detail, but consider the example above that it should include the log2(n_acc) where n_acc is the number of items accumulated in one accumulator for that layer type. But it will also need to include the precision of the input and weight in the case of a Dense or Conv layer, for example. I think it's like weight_bits + input_bits - 1 for the width and weight_integer_bits + input_integer_bits - 1 for the integer part for a multiplier output.

First off, the dictionary returned by the existing config utility should include an entry for the accumulator for layers that have one, for example here. It's referred to as 'accum' in the config. Then I think you can define a new method , like set_accumulator_types(model, config) that can replace those config entries with the appropriate types based on the other precisions and layer types, with some maximum value.

Does that sound sensible to the others too? @jmduarte @zhuangh @vloncar ?

[zhuangh]

hi @thesps, thanks. It looks good to me. FYI the corresponding logic in qkeras is here

Regarding the accumulator type from data, one question I has been wondering is how to make sure the profiling data is representative enough?

[zhuangh]

could you take a look at that? I think we didn't discuss it before, but as well as setting the layer output from the sampled data you could derive the required accumulator type given the data, which can also be set in the configuration dictionary.

Thank you for tagging me. I'm currently looking into making set_data_types_from_keras_model() set accum_t of a layer to the same type as the layer output one (e.g. if the method infers that fc1 should have result precision ap_fixed<11,2>, its accum_t will also be ap_fixed<11,2>). The implementation I'm currently testing does exactly that and it seems to resolve the QKeras performance issue.

Is there motivation for accum_t to be different than the actual layer output precision?

Hi @maksgraczyk sorry, I do not have the context here, what is the QKeras performance issue? Just curious.

[maksgraczyk]

Hi @maksgraczyk sorry, I do not have the context here, what is the QKeras performance issue? Just curious.

I just mean the hls4ml issue we're discussing here. I wrote QKeras there since this specific case happens on a QKeras model.

[zhuangh]

Hi @maksgraczyk sorry, I do not have the context here, what is the QKeras performance issue? Just curious.

I just mean the hls4ml issue we're discussing here. I wrote QKeras there since this specific case happens on a QKeras model.

I see, thanks for the quick response!

[maksgraczyk]

I've updated #321 to make set_data_types_from_keras_model() update accum_t as well and add set_accum_from_keras_model() setting accum_t using just a Keras model and HLSModel data types (without using profiling information). This should also solve the issue here (although not optimally) as it rolls back to using default precision for accum_t in relevant layers. Comments and feedback are welcome.

[thesps]

Thanks for the quick turnaround. I gave set_accum_from_keras_model a quick try on the hls4ml tutorial part 4 QKeras model, and the previous accuracy loss is recovered 👍 and further, the accumulator types are narrower than the old implicit default ap_fixed<16,6> where they can be.
For the part 1 float Keras model, as expected the generated accumulator types are quite wide.

On this comment:

The function supports Dense-type layers only. If your model contains other layers with accum_t (e.g. convolutional
layers), consider using set_data_types_from_keras_model() instead.

Adding support for other layer types is the next step. We would need Conv and AveragePooling support as well. For Conv the only thing that differs is the n. For AveragePooling you can also take into account that there is no multiplication that happens, you're only summing over some inputs.

[zhuangh]

Adding support for other layer types is the next step. We would need Conv and AveragePooling support as well. For Conv the only thing that differs is the n. For AveragePooling you can also take into account that there is no multiplication that happens, you're only summing over some inputs.

Hi @thesps in AveragePooling, there is pool_size for the average operation, therefore, the precision is of interest.