Add quantized sigmoid, fix quantized tanh for QKeras

[jmitrevs]

This is an attempt to fix quantized tanh and add quantized sigmoid activation support for QKeras ingestion. In Quartus the tanh handling probably needs an optimizer to rename it to dense_tanh (and that is why the tests fail--the hacky fix in the writer doesn't work), but for Vivado I think this generally works. Overall, though, I think this fix is only temporary. The method breaks if granularity='name' is not used in hls4ml.utils.config_from_keras_model, though this feature is not just for these quantizers.

Any suggestions?

[thesps]

The method breaks if granularity='name' is not used in hls4ml.utils.config_from_keras_model, though this feature is not just for these quantizers.

You could move the new logic that sets the number of bits and rounding mode to an optimizer pass instead

[jmitrevs]

I agree about moving the logic to an optimizer pass, but that would be for all the quantizers, not just for quantized tanh and sigmoid. Basically all QKeras info is lost if granularity='name' is not used, so it is a more fundamental change we should discuss.

[thesps]

Also can you push to a local branch (like pr/569) to trigger Gitlab CI for pytests?

[jmitrevs]

Let me do a few more tests first, then I'll push to a local branch.

[jmitrevs]

The issue that we are seeing is that the hls4ml sigmoid really seems to approximate 1/1+exp(-x), but the QKeras one is quite different in a way I don't currently understand.

[jmitrevs]

So qkeras uses the hard sigmoid by default: https://github.com/google/qkeras/blob/master/qkeras/quantizers.py#L192-L237 I have to see if hls4ml implements it; otherwise, it's easy to do: clip(0.5 * x + 0.5, 0.0, 1.0).

[jmitrevs]

This resulted in more changes than I expected. Please take a look to see if this is reasonable.

[jmitrevs]

I am not sure how the matplotlib import came into this pull request. It should not be there. I will fix that.

[jmitrevs]

Merged in master to fix pytest setup. Not sure why the tests aren't starting, though.

[jmitrevs]

I pushed the changes to pr/569

[thesps]

I notice in the qkeras pytest we get: E TypeError: __init__() got an unexpected keyword argument 'use_real_tanh' It's because that option is in QKeras main branch but not in a release.

[thesps]

So a few tests are still failing, including the existing hard_sigmoid tests in test_activations, some of which fail and at least one of which now creates a segmentation fault. The test with these settings produces the segfault:

backend, shape, io_type, activation, name = 'Quartus', (8, ), 'io_stream', Activation('hard_sigmoid'), 'hard_sigmoid'

Apart from that, I think we should remove the CNN fixes to a separate PR to keep this one on topic.

[jmitrevs]

The CNN fixes are actually in #610. You are right that it should not be here. I am not sure if it's best to try to get that in first or to revert the commit.

[jmitrevs]

I fixed up #610 so maybe it's easier to merge this after that PR (and I fix the remaining issues).

[jmitrevs]

I think this is ready, but I would like to stage it after #610 (which is also ready). That way the encoded fix, that's partially included here, does not need to be removed. After #610 is merged, I would need to glance one more time at this PR.

[jmitrevs]

Somehow neither on my mac nor on correlator4 do I get a pytest error for qkeras. I will check package versions.

[vloncar]

Could it be an issue of random seed? Maybe you can reproduce if you pass extra parameters, like here. That --randomly-seed=42 parameter may be enough.

[jmitrevs]

It still passes with those values. I do wonder if it's the qkeras version. I noticed after doing git pull on the master branch the pytests no longer work for unrelated reasons (TypeError: Got an unexpected keyword argument 'use_legacy_format'), and checkout out v0.9.0 fails for compatibility reasons. I probably need to target v0.9.0 and make sure that the tests succeed with that version (and disable quantized sigmoid, which is not supported in that version.)

[vloncar]

use_legacy_format comes from keras serialization functions. You need the latest TF installation for QKeras main. That's a bummer, we need them to make a release and fix on that. 0.9.0 is getting a bit old.

[jmitrevs]

So the test fails with tensorflow 2.4.1 (used by the CI) but passes with 2.9.1 and 2.11.0. I don't know why. Should I increase the allowed difference?

[jmitrevs]

The pytests now succeed. I am not sure why there are the CI failures, though. Is it because we couldn't build the docker image with Vivado?

[vloncar]

Looks like a real error, probably something was generated with a syntax error.

[jmitrevs]

I was able to manually reproduce it. It is in the vivado_accelerator_writer not liking the pre-commit changes:

            elif 'void myproject(' in line:
                newline = f'void {model.config.get_project_name()}_axi(\n'

This basically removes that arguments that were added to the same line by the pre-commit:

void myproject(input_axi_t in[N_IN], output_axi_t out[N_OUT]);

The fix should be easy. I'll try to fix it also in vivado_writer where it currently doesn't cause a problem but could.