Skip to content

Fix inplace variables #714

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
merged 9 commits into from
Mar 30, 2023
Merged

Conversation

jmitrevs
Copy link
Contributor

@jmitrevs jmitrevs commented Feb 14, 2023

Description

Inplace variables are broken in the main branch because they are fixed too early. Subsequent optimizations can make this connection no longer valid. Issue #707 (and the attempted fix in #708) is an example of this issue. This was largely developed in the ingest-qonnx-master branch, but is extracted here, and an omission related to stream repacking for quartus was discovered and fixed.

Type of change

Bug fix

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change which adds functionality)

Tests

The pytest, test_reshape.py, is the main test, though test_cnn_mnist.py is also pertinent. The new reshape functionality for Intel needs testing, though. There is also the question of whether flatten can always be removed for io_stream. This should be checked further, and if exceptions are found, we may need to change when we run the optimizers.

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. (May want to add another for Flatten. in various cases.)

@jmitrevs jmitrevs added bug please test Trigger testing by creating local PR branch labels Feb 14, 2023
@jmitrevs jmitrevs added this to the v0.7.0 milestone Feb 16, 2023
@jmduarte jmduarte added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Mar 4, 2023
@jmduarte jmduarte self-requested a review March 7, 2023 00:55
@jmitrevs jmitrevs added please test Trigger testing by creating local PR branch and removed please test Trigger testing by creating local PR branch labels Mar 7, 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 Mar 30, 2023
@vloncar vloncar merged commit 7063e87 into fastmachinelearning:main Mar 30, 2023
JanFSchulte pushed a commit to JanFSchulte/hls4ml that referenced this pull request May 23, 2023
* extract inplace variables from qonnx branch

* fix up reshape batch dimension handling

* add repack stream for quartus

* fix typo in HLS pragma

* pre-commint cleanup, and add missing check for pragma not None

* Minor cosmetic fixes

---------

Co-authored-by: Vladimir Loncar <[email protected]>
calad0i pushed a commit to calad0i/hls4ml that referenced this pull request Jul 1, 2023
* extract inplace variables from qonnx branch

* fix up reshape batch dimension handling

* add repack stream for quartus

* fix typo in HLS pragma

* pre-commint cleanup, and add missing check for pragma not None

* Minor cosmetic fixes

---------

Co-authored-by: Vladimir Loncar <[email protected]>
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