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 profiling SeparableConv1D and SeparableConv2D #891

Merged
merged 2 commits into from Oct 20, 2023

Conversation

qberthet
Copy link
Contributor

@qberthet qberthet commented Oct 17, 2023

Description

As described in #890, profiling model using SeparableConv1D or SeparableConv2D fail.
The application of theses two commit fix the test cases decribed in the issue.

  • The base issue is similar to Profiling layers in an RNN #829, which have been fixed by Fix profiling for GRU/LSTM #833. The first commit of this PR contains very similar changes.
  • The second commit fixes a subsequent issue where a list is used where a dict is expected. I am less sure about this one, maybe it hides something more deep, or it fixes another bug not yet reported. Experienced eyes required here.

closes #890

Type of change

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

Tests

Tests explaining how to reproduce the problem have been provided in the original issue #890.

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 added the please test Trigger testing by creating local PR branch label Oct 18, 2023
@qberthet
Copy link
Contributor Author

Just noticed that there is one unwnated merge commit. I'll fix this later today.
The two first commits can still be reviewed.

@jmitrevs jmitrevs 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
@jmitrevs jmitrevs added this to the v0.8.0 milestone Oct 20, 2023
@@ -346,6 +350,7 @@ def activations_keras(model, X, fmt='longform', plot='boxplot'):
outputs = _get_outputs(
[layer for layer in model.layers if not isinstance(layer, keras.layers.InputLayer)], X, model.input
)
outputs = dict(zip([layer.name for layer in model.layers if not isinstance(layer, keras.layers.InputLayer)], outputs))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a nice catch that we missed in the #863

@vloncar vloncar merged commit 62d5e03 into fastmachinelearning:main Oct 20, 2023
7 checks passed
@qberthet qberthet deleted the fix_890 branch October 24, 2023 14:25
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.

Profiling SeparableConv1D and SeparableConv2D layers fail
3 participants