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

Fix Conv2D with io_type = io_parallel & Strategy: Resource #448

Merged
merged 1 commit into from
Nov 10, 2021

Conversation

thesps
Copy link
Contributor

@thesps thesps commented Nov 9, 2021

Currently a Conv2D layer with io_type = io_parallel & Strategy: Resource gives wrong results. The loops of im2col_2d_cl needed reordering.

test_cnn_mnist.py probes the issue. On master branch the problematic combination is excluded, but if we run it we see:

Accuracy keras:      0.9838
Accuracy hls4ml:     0.112
Relative difference: 0.8861557227078675

>     assert acc_keras > 0.95 and rel_diff < 0.01
E     assert (0.9838 > 0.95 and 0.8861557227078675 < 0.01)

test_cnn_mnist.py:72: AssertionError

Now it can be included, passing with:

Accuracy keras:      0.9838
Accuracy hls4ml:     0.9834
Relative difference: 0.0004065867046147143

I've also reduced the number of MNIST images used in the test from 10000 to 5000, since that test was already the slowest and this PR adds another iteration of it.

Solves issue #375 , and also seen in issue #437.

For completeness: this doesn't touch io_stream implementations, they're all fine.
I haven't touched im2col_2d_cf, but is that even accessible?

… relevant test. Use 5000 MNIST samples rather than full dataset for faster testing
@thesps thesps requested a review from vloncar November 9, 2021 17:46
@thesps thesps added this to the v0.6.0 milestone Nov 9, 2021
Copy link
Contributor

@vloncar vloncar left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @thesps

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QKeras bad predictions Fix accuracy test for MNIST CNN with IOType=io_parallel and Strategy=resource
2 participants