Skip to content
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

Make binary CNN match between Keras and hls4ml #804

Merged
merged 19 commits into from
Aug 16, 2023

Conversation

jmitrevs
Copy link
Contributor

@jmitrevs jmitrevs commented Jun 8, 2023

Description

The check on the pytest for binary CNNs had to be disabled because it failed. This enables it, and attempts to fix issues that it discovers

Type of change

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

Tests

The test checking has been enabled. It still fails in most cases.

Checklist

  • I have read the guidelines for contributing.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • 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 June 8, 2023 01:41
@jmitrevs
Copy link
Contributor Author

jmitrevs commented Jun 8, 2023

This now matches for io_parallel implementations with the Latency strategy. Resource still fails (potentially for unrelated reasons also on Quartus) and for io_stream. @vloncar , I am curious if you think there's a better way to preserve the XnorPrecision designation when doing MaxPooling than I am doing here. It is a bit related to precision propagation, but I think in this case you really need to preserve the type for the network to make sense.

@jmitrevs
Copy link
Contributor Author

jmitrevs commented Jun 20, 2023

With the latest push, this works for Vitis and Vivado (but not Quartus). Note that #817 was triggered with the test previously, which has been modified to not trigger the problem. However, a better fix for #817 would be preferred. It is somewhat beyond the scope of this PR, though.

@jmitrevs
Copy link
Contributor Author

The way Winograd is written it doesn't support the xnor types, so I have forced the Quartus io_parallel tests to use im2col. If we want to update Winograd to do binary, I'll leave that as a separate development.

@jmitrevs jmitrevs marked this pull request as ready for review June 22, 2023 00:44
@jmitrevs jmitrevs added bug please test Trigger testing by creating local PR branch labels Jun 22, 2023
@jmitrevs
Copy link
Contributor Author

Don't fully understand why the pytests fail here but succeeded on my laptop. I thought I was using the same random number, but maybe not.

@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Jul 6, 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 Jul 7, 2023
@jmitrevs
Copy link
Contributor Author

jmitrevs commented Jul 7, 2023

I think the issue was just accumulator precision in the test (big number - almost equal big number) and then quantization based on > or < 0. Let's see if increasing the bitwidth makes the tests succeed.

@jmitrevs jmitrevs requested a review from vloncar July 8, 2023 16:21
@jmitrevs jmitrevs added this to the v0.8.0 milestone Jul 14, 2023
Copy link
Contributor

@bo3z bo3z left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me - main comments on the test setup, that was left-over from the previous changes.

test/pytest/test_binary_cnn.py Outdated Show resolved Hide resolved
test/pytest/test_binary_cnn.py Outdated Show resolved Hide resolved
test/pytest/test_binary_cnn.py Outdated Show resolved Hide resolved
test/pytest/test_binary_cnn.py Show resolved Hide resolved
@bo3z
Copy link
Contributor

bo3z commented Jul 19, 2023

The way Winograd is written it doesn't support the xnor types, so I have forced the Quartus io_parallel tests to use im2col. If we want to update Winograd to do binary, I'll leave that as a separate development.

Enabling binary support for Winograd needs to change (i) the explicit multiplication for binary weights, to use the product function from nnet_mult and (ii) for binary inputs, see how the addition of input elements behaves. But Winograd's algorithm is really meant to reduce the number of multiplications/DSP utilisation and with binary CNNs, that's not really a concern, so I don't think it is necessarily important in this case.

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

jmitrevs commented Aug 8, 2023

I see there are some errors. Will make it draft till I fix them.

@jmitrevs jmitrevs marked this pull request as draft August 8, 2023 22:33
@jmitrevs jmitrevs marked this pull request as ready for review August 10, 2023 00:48
@jmitrevs
Copy link
Contributor Author

The failures seem to be due to precision of the quantization, so I am removing the draft designation

@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Aug 10, 2023
@bo3z
Copy link
Contributor

bo3z commented Aug 11, 2023

Looks good to me.

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

The test passed, so I think we can maybe merge this.

@vloncar vloncar merged commit cecf3e7 into fastmachinelearning:main Aug 16, 2023
4 checks passed
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.

3 participants