Skip to content

fix conv1d io_parallel resource #403

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

Conversation

jmitrevs
Copy link
Contributor

With this fix conv_1d_resource_cl produces the same results as conv_1d_latency_cl and also io_stream versions of conv_1d. I will test it a bit more thoroughly tomorrow. The final fix we should also cherry-pick into the v0.6.0 candidate branch.

@jmitrevs jmitrevs added the bug label Sep 28, 2021
@thesps
Copy link
Contributor

thesps commented Sep 28, 2021

Thanks for this! I pushed the PR branch to a local one (pr/403) to run the tests. We should include a test somewhat similar to the current test_keras_mnist.py to test a Conv1D model with the variations: Strategy: {Resource, Latency}, IOType: {io_parallel, io_stream} and check for functional correctness. But it could be a synthetic model rather than a trained one.

Regarding the implementation of the fix, I see you reversed the order of the loops in the HLS. Did you check that the transpose that we do of the weights is actually the correct one?

@jmitrevs
Copy link
Contributor Author

I looked at the transpose of the weights but I didn't consider changing it because I thought io_stream resource might use it, and io_stream resource seemed to work fine. I didn't look in any detail, though.

@jmduarte jmduarte requested review from jmduarte and thesps September 29, 2021 04:15
Copy link
Contributor

@thesps thesps left a comment

Choose a reason for hiding this comment

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

I implemented a Conv1D test that checks the hls4ml model accuracy relative to the Keras original for [io_parallel, io_stream] x [resource, latency].
It fails on master branch for (io_parallel, resource) but they all pass with this PR.

@thesps thesps merged commit 4564916 into fastmachinelearning:master Sep 30, 2021
thesps added a commit that referenced this pull request Sep 30, 2021
* fix conv1d io_parallel resource

* replace tabs with spaces in modified code

* Add test for Conv1D example model

Co-authored-by: Sioni Summers <[email protected]>
calad0i pushed a commit to calad0i/hls4ml that referenced this pull request Jul 1, 2023
* fix conv1d io_parallel resource

* replace tabs with spaces in modified code

* Add test for Conv1D example model

Co-authored-by: Sioni Summers <[email protected]>
@jmitrevs jmitrevs deleted the fix_conv1d_io_parallel_resource branch February 7, 2025 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants