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 config structure name in pragma for SeparableConv1D #884

Merged
merged 5 commits into from Oct 20, 2023

Conversation

qberthet
Copy link
Contributor

@qberthet qberthet commented Oct 10, 2023

Description

Simple fix for #883

closes #883

Type of change

For a new feature or function, please create an issue first to discuss it
with us before submitting a pull request.

Note: Please delete options that are not relevant.

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

Tests

Tested with example code provided in bug report #883 : Fail before applying this commit, works afterward.

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.

@qberthet qberthet mentioned this pull request Oct 10, 2023
4 tasks
@jmitrevs jmitrevs added the please test Trigger testing by creating local PR branch label Oct 18, 2023
@jmitrevs
Copy link
Contributor

Is it possible to add a pytest to show that there is a problem and that this fixes it? We don't do synthesis in pytests but we do compile the HLS code.

@qberthet
Copy link
Contributor Author

I'll try to add this later today.

@@ -23,7 +23,7 @@ void depthwise_product(data_T data[CONFIG_T::kernel_size * CONFIG_T::n_chan], re

#pragma HLS ARRAY_PARTITION variable=mult complete

#pragma HLS ALLOCATION operation instances=mul limit=CONFIG_T::multiplier_limit
#pragma HLS ALLOCATION operation instances=mul limit=CONFIG_T::mult_config::multiplier_limit
Copy link
Contributor

Choose a reason for hiding this comment

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

This technically works, but we deliberately didn't use the mult_config in depthwise_product. In 2D the config has the multiplier_limit and we should use the same approach. So below this line we should add to these lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I was suspecting that there was something else to it, but not familiar with the code base yet. I'll start by adding a pytest that trig the issue, add the fix you suggest and force push to this PR.

@qberthet
Copy link
Contributor Author

qberthet commented Oct 19, 2023

@jmitrevs : I've added a test, but the problem only appear during CSynth, not CSim, so not sure if that will be ok (requires vivado). Also had to catch the return code of vivado_hls command to raise exception and make the test fail.
I've duplicated sepconv2d test for sepconv1d and then added a test for the build step.
Let me know if you prefer that I remove the tree first commits to only have the fix.

@vloncar : your fix obviously does the job. Thanks!

@qberthet
Copy link
Contributor Author

And I just realize that my test is not setting the output folder correctly. Let me know if you want to keep these tests before I fix it.

@jmitrevs jmitrevs added this to the v0.8.0 milestone Oct 20, 2023
@vloncar vloncar added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Oct 20, 2023
@vloncar vloncar merged commit d035738 into fastmachinelearning:main Oct 20, 2023
5 of 7 checks passed
@qberthet qberthet deleted the fix_883 branch October 24, 2023 14:29
calad0i added a commit to calad0i/hls4ml that referenced this pull request Nov 7, 2023
🎉 Add proxy_model support

format

Fix broken import

Add parallelization_factor propagation

Add overriding warning

Purge linear layers for proxy model configured layers

Fix repack_stream optimizer inheirt original precision

format not-my-code

add handler, fix type enforcement

Speed up Keras profiling (fastmachinelearning#863)

* Speed up Keras profiling

* update function name

---------

Co-authored-by: Javier Duarte <jduarte@ucsd.edu>

[pre-commit.ci] pre-commit autoupdate

updates:
- [github.com/pre-commit/pre-commit-hooks: v4.4.0 → v4.5.0](pre-commit/pre-commit-hooks@v4.4.0...v4.5.0)
- [github.com/asottile/pyupgrade: v3.14.0 → v3.15.0](asottile/pyupgrade@v3.14.0...v3.15.0)

Fix profiling SeparableConv1D and SeparableConv2D (fastmachinelearning#891)

* Profiling: Fix suffixes for SeparableConv1D&2D

* Profiling: transform list to dict where dict is expected

---------

Co-authored-by: Quentin Berthet <quentin.berthet@cern.ch>

Add support for filt_height==1 for streaming quartus conv2d (fastmachinelearning#886)

Fix config structure name in pragma for SeparableConv1D (fastmachinelearning#884)

* Raise exception if Vivado command fail

* Duplicate sepconv2d test for sepconv1d

* Test that csynth is working for sepconv1d

* Define multiplier_limit in nnet::conv1d_config (for sepconv1d)

* Revert build test

---------

Co-authored-by: Quentin Berthet <quentin.berthet@cern.ch>
Co-authored-by: Vladimir Loncar <vloncar@users.noreply.github.com>

[pre-commit.ci] pre-commit autoupdate

updates:
- [github.com/psf/black: 23.9.1 → 23.10.0](psf/black@23.9.1...23.10.0)

rename, minor bug fix

fix multi clones w/ diff outs in stream io

fix test

Fix quartus writer with io_stream and multi-output

fix weight fp write length

add test

Fix: update weight writer digits

Fix clone precision inheriting

Add relu6 stream support

Fix semi-heterogeneous mask generation

Add test and fixed_point_quantizer

format

Revert "Add relu6 stream support"

This reverts commit d05cbaa.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
please test Trigger testing by creating local PR branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SeparableConv1d fail to synthesize
3 participants