Skip to content

CNNs with binary inputs and weights need fixes #749

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

Merged
merged 11 commits into from
Apr 24, 2023

Conversation

jmitrevs
Copy link
Contributor

@jmitrevs jmitrevs commented Apr 6, 2023

Description

As shown by #740, CNNs with binary quantizers don't currently work properly. This PR attempts to fix it.

Type of change

  • Bug fix (non-breaking change that fixes an issue)

Tests

A pytest is added. However, it still shows errors in streaming.

Checklist

  • I have read the guidelines for contributing.
  • I have commented my code, particularly in hard-to-understand areas.
  • My changes generate no new warnings.
  • I have installed and run pre-commit on the files I edited or added.
  • I have added tests that prove my fix is effective or that my feature works.

@jmitrevs jmitrevs marked this pull request as draft April 6, 2023 22:37
@jmduarte jmduarte linked an issue Apr 6, 2023 that may be closed by this pull request
@jmitrevs
Copy link
Contributor Author

jmitrevs commented Apr 7, 2023

Before I pushed 7186346 the test results were:

test_binary_cnn.py::test_model2[io_parallel-Vivado] PASSED                                                    [ 16%]
test_binary_cnn.py::test_model2[io_stream-Quartus] FAILED                                                     [ 33%]
test_binary_cnn.py::test_model2[io_stream-Vivado] FAILED                                                      [ 50%]
test_binary_cnn.py::test_model2[io_stream-Vitis] FAILED                                                       [ 66%]
test_binary_cnn.py::test_model2[io_parallel-Quartus] FAILED                                                   [ 83%]
test_binary_cnn.py::test_model2[io_parallel-Vitis] PASSED                                                     [100%]

After, they all fail. So it was not a successful fix.

@jmduarte jmduarte added the bug label Apr 10, 2023
@jmduarte jmduarte added the please test Trigger testing by creating local PR branch label Apr 11, 2023
@jmitrevs jmitrevs marked this pull request as ready for review April 14, 2023 04:28
@jmitrevs jmitrevs added this to the v0.7.0 milestone Apr 14, 2023
@jmitrevs
Copy link
Contributor Author

This is not complete, and the final test has been disabled, but I think the fixes that we have here should be added before we make 0.7.0 RC. The other stuff we can add after the RC is built or later.

The remaining to-dos:

  1. Max Pooling does not propagate XnorPrecisionType from the input to the output, so the logic is confused. The output zeros are no longer considered to be -1 but just 0.
  2. Conv2d, io_parallel, Resource fails badly with binary inputs--the problem has not been debugged carefully.

If this is accepted, those to-dos should be moved to an issue.

@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Apr 14, 2023
@jmitrevs
Copy link
Contributor Author

This fixes a few things:

  1. cast is called with the correct mult_config in CNN cases in all cases. Previously it was correct in many cases but not all.
  2. cast was fixed in the quartus case since you cannot cast ac_fixed to ac_int without calling .to_ac_int(). Maybe one needs to add more templates here depending on whether the input types are ac_fixed or ac_int to cover all the options. (I understand ac_int to ac_fixed casting is allowed.) It's worth looking in more detail since we probably do not cover all cases.
  3. The binary versions of batchnorm did not make use of n_scale_bias, which is not the same as n_in when n_filt != -1. They have been updated to match the regular batchnorm. (This was also coped to quartus, where it was missing everywhere.)

@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Apr 21, 2023
@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Apr 24, 2023
@vloncar vloncar merged commit 856e778 into fastmachinelearning:main Apr 24, 2023
JanFSchulte pushed a commit to JanFSchulte/hls4ml that referenced this pull request May 23, 2023
* fix cast in remaining places for binary CNNs

* add pytest for binary cnn

* attempted fix for streaming normalize_binary_tanh and normalize_ternary_tanh

* make all compile, though test differences are still too large

* update pytest, disable comparison for now

* remove setting of precision in max pool

* specify the full path out test output
calad0i pushed a commit to calad0i/hls4ml that referenced this pull request Jul 1, 2023
* fix cast in remaining places for binary CNNs

* add pytest for binary cnn

* attempted fix for streaming normalize_binary_tanh and normalize_ternary_tanh

* make all compile, though test differences are still too large

* update pytest, disable comparison for now

* remove setting of precision in max pool

* specify the full path out test output
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug please test Trigger testing by creating local PR branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CNNs with binary quantizer fail
3 participants